Sharing backend: shares CRUD + ACL enforcement everywhere (#16) #35
@@ -84,6 +84,12 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
|
||||
gardens.GET("/:id/full", h.getGardenFull) // one-shot editor load
|
||||
gardens.POST("/:id/objects", h.createObject)
|
||||
|
||||
// Sharing (owner-managed; a recipient may remove their own share).
|
||||
gardens.GET("/:id/shares", h.listShares)
|
||||
gardens.POST("/:id/shares", h.addShare)
|
||||
gardens.PATCH("/:id/shares/:userId", h.updateShare)
|
||||
gardens.DELETE("/:id/shares/:userId", h.removeShare)
|
||||
|
||||
// Objects are addressed by their own id; the service resolves the owning
|
||||
// garden for the permission check.
|
||||
objects := v1.Group("/objects", h.requireAuth())
|
||||
|
||||
@@ -30,6 +30,12 @@ func writeServiceError(c *gin.Context, err error) {
|
||||
writeAPIError(c, http.StatusConflict, "VERSION_CONFLICT", "the resource was modified; refetch and retry")
|
||||
case errors.Is(err, domain.ErrPlantInUse):
|
||||
writeAPIError(c, http.StatusConflict, "PLANT_IN_USE", "this plant is used by plantings and can't be deleted")
|
||||
case errors.Is(err, domain.ErrShareUserNotFound):
|
||||
writeAPIError(c, http.StatusNotFound, "SHARE_USER_NOT_FOUND", "no account with that email")
|
||||
case errors.Is(err, domain.ErrCannotShareWithSelf):
|
||||
writeAPIError(c, http.StatusBadRequest, "CANNOT_SHARE_WITH_SELF", "you can't share a garden with yourself")
|
||||
case errors.Is(err, domain.ErrShareExists):
|
||||
writeAPIError(c, http.StatusConflict, "SHARE_EXISTS", "this garden is already shared with that user")
|
||||
case errors.Is(err, domain.ErrInvalidCredentials):
|
||||
writeAPIError(c, http.StatusUnauthorized, "INVALID_CREDENTIALS", "invalid email or password")
|
||||
case errors.Is(err, domain.ErrEmailTaken):
|
||||
|
||||
@@ -0,0 +1,88 @@
|
||||
package api
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
|
||||
"github.com/gin-gonic/gin"
|
||||
)
|
||||
|
||||
// shareCreateRequest is the body for POST /gardens/:id/shares: the recipient's
|
||||
// exact account email and the role to grant (viewer|editor).
|
||||
type shareCreateRequest struct {
|
||||
Email string `json:"email" binding:"required"`
|
||||
Role string `json:"role" binding:"required"`
|
||||
}
|
||||
|
||||
// shareUpdateRequest is the body for PATCH /gardens/:id/shares/:userId.
|
||||
type shareUpdateRequest struct {
|
||||
Role string `json:"role" binding:"required"`
|
||||
}
|
||||
|
||||
func (h *handlers) listShares(c *gin.Context) {
|
||||
gardenID, ok := parseIDParam(c, "id")
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
shares, err := h.svc.ListShares(c.Request.Context(), mustActor(c).ID, gardenID)
|
||||
if err != nil {
|
||||
writeServiceError(c, err)
|
||||
return
|
||||
}
|
||||
c.JSON(http.StatusOK, shares)
|
||||
}
|
||||
|
||||
func (h *handlers) addShare(c *gin.Context) {
|
||||
gardenID, ok := parseIDParam(c, "id")
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
var req shareCreateRequest
|
||||
if err := c.ShouldBindJSON(&req); err != nil {
|
||||
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "an email and a role are required")
|
||||
return
|
||||
}
|
||||
sh, err := h.svc.AddShare(c.Request.Context(), mustActor(c).ID, gardenID, req.Email, req.Role)
|
||||
if err != nil {
|
||||
writeServiceError(c, err)
|
||||
return
|
||||
}
|
||||
c.JSON(http.StatusCreated, sh)
|
||||
}
|
||||
|
||||
func (h *handlers) updateShare(c *gin.Context) {
|
||||
gardenID, ok := parseIDParam(c, "id")
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
userID, ok := parseIDParam(c, "userId")
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
var req shareUpdateRequest
|
||||
if err := c.ShouldBindJSON(&req); err != nil {
|
||||
writeAPIError(c, http.StatusBadRequest, "INVALID_INPUT", "a role is required")
|
||||
return
|
||||
}
|
||||
sh, err := h.svc.UpdateShareRole(c.Request.Context(), mustActor(c).ID, gardenID, userID, req.Role)
|
||||
if err != nil {
|
||||
writeServiceError(c, err)
|
||||
return
|
||||
}
|
||||
c.JSON(http.StatusOK, sh)
|
||||
}
|
||||
|
||||
func (h *handlers) removeShare(c *gin.Context) {
|
||||
gardenID, ok := parseIDParam(c, "id")
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
userID, ok := parseIDParam(c, "userId")
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
if err := h.svc.RemoveShare(c.Request.Context(), mustActor(c).ID, gardenID, userID); err != nil {
|
||||
writeServiceError(c, err)
|
||||
return
|
||||
}
|
||||
c.Status(http.StatusNoContent)
|
||||
}
|
||||
@@ -0,0 +1,91 @@
|
||||
package api
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"strconv"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func sharesPath(gid int64) string { return "/api/v1/gardens/" + strconv.FormatInt(gid, 10) + "/shares" }
|
||||
func sharePath(gid, userID int64) string {
|
||||
return sharesPath(gid) + "/" + strconv.FormatInt(userID, 10)
|
||||
}
|
||||
|
||||
func TestSharingFlowAPI(t *testing.T) {
|
||||
r := authEngine(t, localCfg())
|
||||
a := registerAndCookie(t, r, "[email protected]")
|
||||
b := registerAndCookie(t, r, "[email protected]")
|
||||
gid := createGardenAPI(t, r, a, "A's yard")
|
||||
|
||||
// A bed to mutate later.
|
||||
w := doJSON(t, r, http.MethodPost, objectsPath(gid),
|
||||
map[string]any{"kind": "bed", "xCm": 500, "yCm": 500, "widthCm": 200, "heightCm": 200}, a)
|
||||
oid := int64(decodeMap(t, w.Body.Bytes())["id"].(float64))
|
||||
|
||||
// Unknown email → 404 SHARE_USER_NOT_FOUND.
|
||||
w = doJSON(t, r, http.MethodPost, sharesPath(gid), map[string]any{"email": "[email protected]", "role": "viewer"}, a)
|
||||
if w.Code != http.StatusNotFound {
|
||||
t.Fatalf("share unknown email = %d, want 404 (body %s)", w.Code, w.Body.String())
|
||||
}
|
||||
|
||||
// Share with B as viewer → 201.
|
||||
w = doJSON(t, r, http.MethodPost, sharesPath(gid), map[string]any{"email": "[email protected]", "role": "viewer"}, a)
|
||||
if w.Code != http.StatusCreated {
|
||||
t.Fatalf("share viewer = %d, body %s", w.Code, w.Body.String())
|
||||
}
|
||||
bID := int64(decodeMap(t, w.Body.Bytes())["userId"].(float64))
|
||||
|
||||
// B lists → the garden with myRole viewer.
|
||||
w = doJSON(t, r, http.MethodGet, "/api/v1/gardens", nil, b)
|
||||
var list []map[string]any
|
||||
if err := json.Unmarshal(w.Body.Bytes(), &list); err != nil {
|
||||
t.Fatalf("decode b list: %v", err)
|
||||
}
|
||||
if len(list) != 1 || list[0]["myRole"] != "viewer" {
|
||||
t.Fatalf("b list = %+v, want one garden with myRole viewer", list)
|
||||
}
|
||||
|
||||
// B reads /full (200) but can't mutate (403) or manage shares (403).
|
||||
if w := doJSON(t, r, http.MethodGet, fullPath(gid), nil, b); w.Code != http.StatusOK {
|
||||
t.Errorf("b read full = %d, want 200", w.Code)
|
||||
}
|
||||
if w := doJSON(t, r, http.MethodPatch, objectPath(oid), map[string]any{"xCm": 100, "version": 1}, b); w.Code != http.StatusForbidden {
|
||||
t.Errorf("viewer mutate object = %d, want 403", w.Code)
|
||||
}
|
||||
if w := doJSON(t, r, http.MethodGet, sharesPath(gid), nil, b); w.Code != http.StatusForbidden {
|
||||
t.Errorf("viewer list shares = %d, want 403", w.Code)
|
||||
}
|
||||
|
||||
// A upgrades B to editor.
|
||||
if w := doJSON(t, r, http.MethodPatch, sharePath(gid, bID), map[string]any{"role": "editor"}, a); w.Code != http.StatusOK {
|
||||
t.Fatalf("upgrade role = %d, body %s", w.Code, w.Body.String())
|
||||
}
|
||||
|
||||
// B can now move the object, but still can't rename the garden (owner-only).
|
||||
if w := doJSON(t, r, http.MethodPatch, objectPath(oid), map[string]any{"xCm": 100, "version": 1}, b); w.Code != http.StatusOK {
|
||||
t.Errorf("editor move object = %d, want 200 (body %s)", w.Code, w.Body.String())
|
||||
}
|
||||
if w := doJSON(t, r, http.MethodPatch, gardenPath(gid),
|
||||
map[string]any{"name": "hijack", "widthCm": 100, "heightCm": 100, "unitPref": "metric", "version": 1}, b); w.Code != http.StatusForbidden {
|
||||
t.Errorf("editor rename garden = %d, want 403", w.Code)
|
||||
}
|
||||
|
||||
// A removes the share → B loses access.
|
||||
if w := doJSON(t, r, http.MethodDelete, sharePath(gid, bID), nil, a); w.Code != http.StatusNoContent {
|
||||
t.Fatalf("remove share = %d", w.Code)
|
||||
}
|
||||
if w := doJSON(t, r, http.MethodGet, fullPath(gid), nil, b); w.Code != http.StatusNotFound {
|
||||
t.Errorf("b read after unshare = %d, want 404", w.Code)
|
||||
}
|
||||
if w := doJSON(t, r, http.MethodGet, "/api/v1/gardens", nil, b); w.Body.String() != "[]" {
|
||||
t.Errorf("b list after unshare = %s, want []", w.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
func TestSharesRequireAuth(t *testing.T) {
|
||||
r := authEngine(t, localCfg())
|
||||
if w := doJSON(t, r, http.MethodGet, "/api/v1/gardens/1/shares", nil, nil); w.Code != http.StatusUnauthorized {
|
||||
t.Errorf("unauthenticated list shares = %d, want 401", w.Code)
|
||||
}
|
||||
}
|
||||
@@ -21,6 +21,17 @@ var (
|
||||
// reference it (the FK is ON DELETE RESTRICT). Mapped to 409.
|
||||
ErrPlantInUse = errors.New("plant is referenced by plantings")
|
||||
|
||||
// ErrShareUserNotFound means no account exists for the email a share invite
|
||||
// targeted (v1 shares an existing user only — no invitation emails). Mapped to
|
||||
// 404 with a clear "no account with that email" message.
|
||||
ErrShareUserNotFound = errors.New("no account with that email")
|
||||
// ErrCannotShareWithSelf means the owner tried to share a garden with their
|
||||
// own account. Mapped to 400.
|
||||
ErrCannotShareWithSelf = errors.New("cannot share a garden with yourself")
|
||||
// ErrShareExists means the garden is already shared with that user. Mapped to
|
||||
// 409 (change the existing share's role instead).
|
||||
ErrShareExists = errors.New("garden already shared with that user")
|
||||
|
||||
// ErrInvalidInput means the caller supplied structurally invalid data (empty
|
||||
// required field, malformed value). Mapped to 400.
|
||||
ErrInvalidInput = errors.New("invalid input")
|
||||
@@ -60,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"
|
||||
@@ -116,6 +130,10 @@ type Garden struct {
|
||||
Version int64 `json:"version"`
|
||||
CreatedAt string `json:"createdAt"`
|
||||
UpdatedAt string `json:"updatedAt"`
|
||||
|
||||
// MyRole is the requesting actor's effective role on this garden ("owner",
|
||||
// "editor", or "viewer"). Computed by the service, never persisted.
|
||||
MyRole string `json:"myRole,omitempty"`
|
||||
}
|
||||
|
||||
// GardenShare grants a non-owner user viewer or editor access to a garden.
|
||||
@@ -130,6 +148,14 @@ type GardenShare struct {
|
||||
UpdatedAt string `json:"updatedAt"`
|
||||
}
|
||||
|
||||
// ShareWithUser is a garden share plus the target user's identity, for the
|
||||
// sharing UI's list (which shows who a garden is shared with).
|
||||
type ShareWithUser struct {
|
||||
GardenShare
|
||||
Email string `json:"email"`
|
||||
DisplayName string `json:"displayName"`
|
||||
}
|
||||
|
||||
// GardenObject is any placeable object in a garden (bed, container, tree, path…).
|
||||
// Positioned by center point + rotation about center, in garden space.
|
||||
type GardenObject struct {
|
||||
|
||||
@@ -32,6 +32,20 @@ const (
|
||||
roleOwner
|
||||
)
|
||||
|
||||
// String renders a role for the API's Garden.MyRole ("owner"/"editor"/"viewer").
|
||||
func (r gardenRole) String() string {
|
||||
switch r {
|
||||
case roleOwner:
|
||||
return domain.RoleOwner
|
||||
|
|
||||
case roleEditor:
|
||||
return domain.RoleEditor
|
||||
case roleViewer:
|
||||
return domain.RoleViewer
|
||||
default:
|
||||
return ""
|
||||
}
|
||||
}
|
||||
|
||||
// GardenInput is the mutable field set for creating or updating a garden.
|
||||
type GardenInput struct {
|
||||
Name string
|
||||
@@ -54,23 +68,41 @@ func (s *Service) requireGardenRole(ctx context.Context, actorID, gardenID int64
|
||||
if err != nil {
|
||||
return nil, err // ErrNotFound or a real error
|
||||
}
|
||||
role := effectiveGardenRole(actorID, g)
|
||||
role, err := s.effectiveGardenRole(ctx, actorID, g)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if role == roleNone {
|
||||
return nil, domain.ErrNotFound
|
||||
}
|
||||
if role < min {
|
||||
return nil, domain.ErrForbidden
|
||||
}
|
||||
g.MyRole = role.String() // so every read-through carries the actor's role
|
||||
return g, nil
|
||||
}
|
||||
|
||||
// effectiveGardenRole is the actor's role on a garden. Owner is implicit via
|
||||
// gardens.owner_id; share-based viewer/editor roles are added in #16.
|
||||
func effectiveGardenRole(actorID int64, g *domain.Garden) gardenRole {
|
||||
// effectiveGardenRole is the actor's role on a garden: owner (implicit via
|
||||
// gardens.owner_id), else a viewer/editor grant from garden_shares, else none.
|
||||
func (s *Service) effectiveGardenRole(ctx context.Context, actorID int64, g *domain.Garden) (gardenRole, error) {
|
||||
if g.OwnerID == actorID {
|
||||
return roleOwner
|
||||
return roleOwner, nil
|
||||
}
|
||||
role, found, err := s.store.GetShareRole(ctx, g.ID, actorID)
|
||||
if err != nil {
|
||||
return roleNone, err
|
||||
}
|
||||
if !found {
|
||||
return roleNone, nil
|
||||
}
|
||||
switch role {
|
||||
case domain.RoleEditor:
|
||||
return roleEditor, nil
|
||||
case domain.RoleViewer:
|
||||
return roleViewer, nil
|
||||
default:
|
||||
return roleNone, nil
|
||||
}
|
||||
return roleNone
|
||||
}
|
||||
|
||||
// CreateGarden creates a garden owned by the actor. Missing dimensions default
|
||||
@@ -81,7 +113,12 @@ func (s *Service) CreateGarden(ctx context.Context, actorID int64, in GardenInpu
|
||||
return nil, err
|
||||
}
|
||||
g.OwnerID = actorID
|
||||
return s.store.CreateGarden(ctx, g)
|
||||
created, err := s.store.CreateGarden(ctx, g)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
created.MyRole = roleOwner.String() // the creator owns it
|
||||
return created, nil
|
||||
}
|
||||
|
||||
// GetGarden returns a garden the actor may at least view.
|
||||
@@ -89,16 +126,18 @@ func (s *Service) GetGarden(ctx context.Context, actorID, gardenID int64) (*doma
|
||||
return s.requireGardenRole(ctx, actorID, gardenID, roleViewer)
|
||||
}
|
||||
|
||||
// ListGardens returns the gardens the actor can see. Owned-only until #16.
|
||||
// ListGardens returns the gardens the actor can see: owned plus shared-with-them,
|
||||
// each row carrying the actor's my_role.
|
||||
func (s *Service) ListGardens(ctx context.Context, actorID int64) ([]domain.Garden, error) {
|
||||
return s.store.ListGardensForOwner(ctx, actorID)
|
||||
return s.store.ListGardensForActor(ctx, actorID)
|
||||
}
|
||||
|
||||
// UpdateGarden applies a version-guarded update; the actor must be at least an
|
||||
// editor. On a version mismatch it returns (current garden, ErrVersionConflict)
|
||||
// so the handler can return the fresh row for the client to rebase.
|
||||
// UpdateGarden applies a version-guarded update to a garden's metadata; only the
|
||||
// OWNER may edit metadata (editors edit contents, not the garden itself). On a
|
||||
// version mismatch it returns (current garden, ErrVersionConflict) so the handler
|
||||
// can return the fresh row for the client to rebase.
|
||||
func (s *Service) UpdateGarden(ctx context.Context, actorID, gardenID int64, in GardenInput, version int64) (*domain.Garden, error) {
|
||||
if _, err := s.requireGardenRole(ctx, actorID, gardenID, roleEditor); err != nil {
|
||||
if _, err := s.requireGardenRole(ctx, actorID, gardenID, roleOwner); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
g, err := gardenFromInput(in, false) // no defaults: an update states every field
|
||||
@@ -107,7 +146,11 @@ func (s *Service) UpdateGarden(ctx context.Context, actorID, gardenID int64, in
|
||||
}
|
||||
g.ID = gardenID
|
||||
g.Version = version
|
||||
return s.store.UpdateGarden(ctx, g)
|
||||
updated, err := s.store.UpdateGarden(ctx, g)
|
||||
if updated != nil {
|
||||
updated.MyRole = roleOwner.String() // only the owner reaches here
|
||||
|
gitea-actions
commented
🟡 UpdateGarden hardcodes roleOwner.String() instead of reusing requireGardenRole result maintainability · flagged by 1 model
🪰 Gadfly · advisory 🟡 **UpdateGarden hardcodes roleOwner.String() instead of reusing requireGardenRole result**
_maintainability · flagged by 1 model_
- `internal/service/gardens.go:151` — `UpdateGarden` hardcodes `updated.MyRole = roleOwner.String()` instead of reusing the role already computed and stamped by `requireGardenRole`. The returned garden from `requireGardenRole` is discarded (`if _, err := ...`), so the permission result is recalculated by hand. This is brittle: if the model ever allows non-owners through (co-owners, admin override), the response will falsely claim `"owner"`. Capture the garden from `requireGardenRole` and use its…
<sub>🪰 Gadfly · advisory</sub>
|
||||
}
|
||||
return updated, err
|
||||
}
|
||||
|
||||
// DeleteGarden removes a garden; only the owner may.
|
||||
|
||||
@@ -121,9 +121,19 @@ func (s *Service) UpdatePlanting(ctx context.Context, actorID, plantingID int64,
|
||||
return nil, err
|
||||
}
|
||||
|
||||
originalPlantID := pl.PlantID
|
||||
applyPlantingPatch(pl, patch)
|
||||
// The plant may have changed; the (possibly new) plant must be visible.
|
||||
plant, err := s.visiblePlant(ctx, actorID, pl.PlantID)
|
||||
// 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
|
||||
|
gitea-actions
commented
🟠 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
🪰 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>
|
||||
// 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 {
|
||||
|
gitea-actions
commented
🟡 Redundant duplicate GetPlant query in UpdatePlanting when the plant is changed error-handling, performance · flagged by 2 models
🪰 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)
|
||||
}
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
@@ -0,0 +1,79 @@
|
||||
package service
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"strings"
|
||||
|
||||
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
|
||||
)
|
||||
|
||||
// isShareRole reports whether role is a grantable share role (owner is implicit,
|
||||
// never a share row).
|
||||
func isShareRole(role string) bool {
|
||||
return role == domain.RoleViewer || role == domain.RoleEditor
|
||||
}
|
||||
|
||||
// ListShares returns a garden's shares (each with the recipient's identity).
|
||||
// Owner only — sharing is managed by the owner alone.
|
||||
func (s *Service) ListShares(ctx context.Context, actorID, gardenID int64) ([]domain.ShareWithUser, error) {
|
||||
if _, err := s.requireGardenRole(ctx, actorID, gardenID, roleOwner); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return s.store.ListSharesForGarden(ctx, gardenID)
|
||||
}
|
||||
|
||||
// AddShare grants a user viewer/editor access to a garden, targeting them by the
|
||||
// exact email of an existing account (v1 has no invitation emails). Owner only.
|
||||
// Unknown email → ErrShareUserNotFound; the owner's own email →
|
||||
// ErrCannotShareWithSelf; an already-shared user → ErrShareExists.
|
||||
func (s *Service) AddShare(ctx context.Context, actorID, gardenID int64, email, role string) (*domain.GardenShare, error) {
|
||||
|
gitea-actions
commented
🟡 AddShare exposes an authenticated user-enumeration oracle (unknown email → 404, known → 201/409) security · flagged by 1 model
🪰 Gadfly · advisory 🟡 **AddShare exposes an authenticated user-enumeration oracle (unknown email → 404, known → 201/409)**
_security · flagged by 1 model_
- `internal/service/shares.go:30` (`AddShare`) — **authenticated user-enumeration oracle.** Any authenticated owner of a garden can probe whether an arbitrary email is a registered pansy account: calling `POST /gardens/:id/shares` with `{"email": "<guess>", "role": "viewer"}` returns `404 SHARE_USER_NOT_FOUND` for an unknown email (mapping at `internal/api/errors.go:33-34`) vs. `201` (or `409 SHARE_EXISTS` per `errors.go:37-38`) for a known one. The owner can then `DELETE` the share to clean up.…
<sub>🪰 Gadfly · advisory</sub>
|
||||
if _, err := s.requireGardenRole(ctx, actorID, gardenID, roleOwner); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if !isShareRole(role) {
|
||||
return nil, domain.ErrInvalidInput
|
||||
}
|
||||
target, err := s.store.GetUserByEmail(ctx, strings.TrimSpace(email))
|
||||
if errors.Is(err, domain.ErrNotFound) {
|
||||
return nil, domain.ErrShareUserNotFound
|
||||
|
gitea-actions
commented
🟡 AddShare 404 for unknown email enables authenticated account enumeration security · flagged by 1 model
🪰 Gadfly · advisory 🟡 **AddShare 404 for unknown email enables authenticated account enumeration**
_security · flagged by 1 model_
- `internal/service/shares.go:39` (`AddShare`) — **authenticated account enumeration.** `GetUserByEmail` returning `domain.ErrNotFound` is converted to `ErrShareUserNotFound`, which `writeServiceError` maps to a distinct 404 `SHARE_USER_NOT_FOUND` with message "no account with that email"; a valid target yields 409/201. Since any authenticated user can create a garden (becoming its owner) and then call `AddShare` on it, *any* logged-in user can probe arbitrary email addresses to learn whether th…
<sub>🪰 Gadfly · advisory</sub>
|
||||
}
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if target.ID == actorID {
|
||||
return nil, domain.ErrCannotShareWithSelf
|
||||
}
|
||||
return s.store.CreateShare(ctx, &domain.GardenShare{
|
||||
GardenID: gardenID, UserID: target.ID, Role: role, CreatedBy: actorID,
|
||||
})
|
||||
}
|
||||
|
||||
// UpdateShareRole changes an existing share's role. Owner only.
|
||||
func (s *Service) UpdateShareRole(ctx context.Context, actorID, gardenID, targetUserID int64, role string) (*domain.GardenShare, error) {
|
||||
if _, err := s.requireGardenRole(ctx, actorID, gardenID, roleOwner); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if !isShareRole(role) {
|
||||
return nil, domain.ErrInvalidInput
|
||||
}
|
||||
return s.store.UpdateShareRole(ctx, gardenID, targetUserID, role)
|
||||
}
|
||||
|
||||
// RemoveShare revokes a share. The garden owner may remove anyone; a recipient
|
||||
// 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
|
||||
|
gitea-actions
commented
🔴 RemoveShare bypasses requireGardenRole authorization choke point maintainability, security · flagged by 5 models
🪰 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 {
|
||||
g, err := s.requireGardenRole(ctx, actorID, gardenID, roleViewer)
|
||||
if err != nil {
|
||||
return err // ErrNotFound for a non-participant
|
||||
|
gitea-actions
commented
🟡 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
🪰 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 {
|
||||
return domain.ErrForbidden
|
||||
}
|
||||
// Owner path removes any share; self-leave removes the actor's own (a missing
|
||||
// row is ErrNotFound either way).
|
||||
return s.store.DeleteShare(ctx, gardenID, targetUserID)
|
||||
}
|
||||
@@ -0,0 +1,205 @@
|
||||
package service
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"testing"
|
||||
|
||||
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
|
||||
)
|
||||
|
||||
// wantErr asserts err matches want (nil = expected success, else errors.Is).
|
||||
func wantErr(t *testing.T, name string, err, want error) {
|
||||
|
gitea-actions
commented
⚪ wantErr helper is local to shares_test.go but generally useful; other tests re-implement the same assertion inline maintainability · flagged by 1 model
🪰 Gadfly · advisory ⚪ **wantErr helper is local to shares_test.go but generally useful; other tests re-implement the same assertion inline**
_maintainability · flagged by 1 model_
- **`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>
|
||||
t.Helper()
|
||||
if want == nil {
|
||||
if err != nil {
|
||||
t.Errorf("%s: got %v, want success", name, err)
|
||||
}
|
||||
return
|
||||
}
|
||||
if !errors.Is(err, want) {
|
||||
t.Errorf("%s: got %v, want %v", name, err, want)
|
||||
}
|
||||
}
|
||||
|
||||
// TestGardenACLMatrix is the authoritative {owner, editor, viewer, stranger} ×
|
||||
// {read, mutate object, mutate planting, edit garden, delete garden, manage
|
||||
// shares} table. Read the columns as: nil = allowed, ErrForbidden = visible but
|
||||
// insufficient, ErrNotFound = existence masked.
|
||||
func TestGardenACLMatrix(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
s := newTestService(t, openConfig())
|
||||
owner := seedUser(t, s, "[email protected]")
|
||||
editor := seedUser(t, s, "[email protected]")
|
||||
viewer := seedUser(t, s, "[email protected]")
|
||||
stranger := seedUser(t, s, "[email protected]")
|
||||
|
||||
g := seedGarden(t, s, owner)
|
||||
bed := seedBed(t, s, owner, g.ID)
|
||||
plant := seedOwnPlant(t, s, owner, 10)
|
||||
plop, err := s.CreatePlanting(ctx, owner, bed.ID, PlantingInput{PlantID: plant.ID, XCM: 0, YCM: 0, RadiusCM: 10})
|
||||
if err != nil {
|
||||
t.Fatalf("seed plop: %v", err)
|
||||
}
|
||||
|
||||
if _, err := s.AddShare(ctx, owner, g.ID, "[email protected]", domain.RoleEditor); err != nil {
|
||||
t.Fatalf("share editor: %v", err)
|
||||
}
|
||||
if _, err := s.AddShare(ctx, owner, g.ID, "[email protected]", domain.RoleViewer); err != nil {
|
||||
t.Fatalf("share viewer: %v", err)
|
||||
}
|
||||
|
||||
// read /full — viewer and up; stranger masked.
|
||||
for _, tc := range []struct {
|
||||
actor int64
|
||||
want error
|
||||
}{{owner, nil}, {editor, nil}, {viewer, nil}, {stranger, domain.ErrNotFound}} {
|
||||
_, err := s.GardenFull(ctx, tc.actor, g.ID)
|
||||
wantErr(t, "readFull", err, tc.want)
|
||||
}
|
||||
|
||||
// mutate object — editor and up. Failure actors fail the role check before any
|
||||
// version check, so a dummy version is fine; success actors need the live one.
|
||||
x := 300.0
|
||||
_, ev := s.UpdateObject(ctx, viewer, bed.ID, ObjectPatch{XCM: &x}, 1)
|
||||
wantErr(t, "mutObject/viewer", ev, domain.ErrForbidden)
|
||||
_, es := s.UpdateObject(ctx, stranger, bed.ID, ObjectPatch{XCM: &x}, 1)
|
||||
wantErr(t, "mutObject/stranger", es, domain.ErrNotFound)
|
||||
cur, _ := s.store.GetObject(ctx, bed.ID)
|
||||
_, ee := s.UpdateObject(ctx, editor, bed.ID, ObjectPatch{XCM: &x}, cur.Version)
|
||||
wantErr(t, "mutObject/editor", ee, nil)
|
||||
cur, _ = s.store.GetObject(ctx, bed.ID)
|
||||
_, eo := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{YCM: &x}, cur.Version)
|
||||
wantErr(t, "mutObject/owner", eo, nil)
|
||||
|
||||
// mutate planting — editor and up.
|
||||
nr := 12.0
|
||||
_, pv := s.UpdatePlanting(ctx, viewer, plop.ID, PlantingPatch{RadiusCM: &nr}, 1)
|
||||
wantErr(t, "mutPlop/viewer", pv, domain.ErrForbidden)
|
||||
_, ps := s.UpdatePlanting(ctx, stranger, plop.ID, PlantingPatch{RadiusCM: &nr}, 1)
|
||||
wantErr(t, "mutPlop/stranger", ps, domain.ErrNotFound)
|
||||
curp, _ := s.store.GetPlanting(ctx, plop.ID)
|
||||
_, pe := s.UpdatePlanting(ctx, editor, plop.ID, PlantingPatch{RadiusCM: &nr}, curp.Version)
|
||||
wantErr(t, "mutPlop/editor", pe, nil)
|
||||
|
||||
// edit garden meta — OWNER only (editors edit contents, not the garden).
|
||||
edit := func(actor, version int64) error {
|
||||
_, err := s.UpdateGarden(ctx, actor, g.ID, GardenInput{Name: "Renamed", WidthCM: 1000, HeightCM: 1000, UnitPref: "metric"}, version)
|
||||
return err
|
||||
}
|
||||
wantErr(t, "editGarden/editor", edit(editor, 1), domain.ErrForbidden)
|
||||
wantErr(t, "editGarden/viewer", edit(viewer, 1), domain.ErrForbidden)
|
||||
wantErr(t, "editGarden/stranger", edit(stranger, 1), domain.ErrNotFound)
|
||||
curg, _ := s.GetGarden(ctx, owner, g.ID)
|
||||
wantErr(t, "editGarden/owner", edit(owner, curg.Version), nil)
|
||||
|
||||
// manage shares (list) — OWNER only.
|
||||
for _, tc := range []struct {
|
||||
actor int64
|
||||
want error
|
||||
}{{editor, domain.ErrForbidden}, {viewer, domain.ErrForbidden}, {stranger, domain.ErrNotFound}, {owner, nil}} {
|
||||
_, err := s.ListShares(ctx, tc.actor, g.ID)
|
||||
wantErr(t, "listShares", err, tc.want)
|
||||
}
|
||||
|
||||
// delete garden — OWNER only. Non-owners are refused (garden survives).
|
||||
wantErr(t, "delGarden/editor", s.DeleteGarden(ctx, editor, g.ID), domain.ErrForbidden)
|
||||
wantErr(t, "delGarden/viewer", s.DeleteGarden(ctx, viewer, g.ID), domain.ErrForbidden)
|
||||
wantErr(t, "delGarden/stranger", s.DeleteGarden(ctx, stranger, g.ID), domain.ErrNotFound)
|
||||
wantErr(t, "delGarden/owner", s.DeleteGarden(ctx, owner, g.ID), nil)
|
||||
}
|
||||
|
||||
func TestListGardensIncludesSharedWithRole(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
s := newTestService(t, openConfig())
|
||||
owner := seedUser(t, s, "[email protected]")
|
||||
other := seedUser(t, s, "[email protected]")
|
||||
g := seedGarden(t, s, owner)
|
||||
|
||||
// Owner sees it as "owner".
|
||||
own, _ := s.ListGardens(ctx, 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.
|
||||
if list, _ := s.ListGardens(ctx, other); len(list) != 0 {
|
||||
t.Fatalf("other list = %d, want 0 before sharing", len(list))
|
||||
}
|
||||
// Share as viewer → other sees it with myRole viewer.
|
||||
if _, err := s.AddShare(ctx, owner, g.ID, "[email protected]", domain.RoleViewer); err != nil {
|
||||
t.Fatalf("share: %v", err)
|
||||
}
|
||||
list, _ := s.ListGardens(ctx, other)
|
||||
if len(list) != 1 || list[0].MyRole != domain.RoleViewer || list[0].ID != g.ID {
|
||||
t.Fatalf("other list after share = %+v, want the garden with myRole viewer", list)
|
||||
}
|
||||
}
|
||||
|
||||
func TestAddShareErrors(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
s := newTestService(t, openConfig())
|
||||
owner := seedUser(t, s, "[email protected]")
|
||||
seedUser(t, s, "[email protected]")
|
||||
g := seedGarden(t, s, owner)
|
||||
|
||||
// Unknown email → ErrShareUserNotFound.
|
||||
if _, err := s.AddShare(ctx, owner, g.ID, "[email protected]", domain.RoleViewer); !errors.Is(err, domain.ErrShareUserNotFound) {
|
||||
t.Errorf("unknown email err = %v, want ErrShareUserNotFound", err)
|
||||
}
|
||||
// Self → ErrCannotShareWithSelf.
|
||||
if _, err := s.AddShare(ctx, owner, g.ID, "[email protected]", domain.RoleViewer); !errors.Is(err, domain.ErrCannotShareWithSelf) {
|
||||
t.Errorf("self-share err = %v, want ErrCannotShareWithSelf", err)
|
||||
}
|
||||
// Bad role → ErrInvalidInput.
|
||||
if _, err := s.AddShare(ctx, owner, g.ID, "[email protected]", "admin"); !errors.Is(err, domain.ErrInvalidInput) {
|
||||
t.Errorf("bad role err = %v, want ErrInvalidInput", err)
|
||||
}
|
||||
// First share ok; a second to the same user → ErrShareExists.
|
||||
if _, err := s.AddShare(ctx, owner, g.ID, "[email protected]", domain.RoleViewer); err != nil {
|
||||
t.Fatalf("first share: %v", err)
|
||||
}
|
||||
if _, err := s.AddShare(ctx, owner, g.ID, "[email protected]", domain.RoleEditor); !errors.Is(err, domain.ErrShareExists) {
|
||||
t.Errorf("double-share err = %v, want ErrShareExists", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestUpdateAndRemoveShare(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
s := newTestService(t, openConfig())
|
||||
owner := seedUser(t, s, "[email protected]")
|
||||
friend := seedUser(t, s, "[email protected]")
|
||||
g := seedGarden(t, s, owner)
|
||||
if _, err := s.AddShare(ctx, owner, g.ID, "[email protected]", domain.RoleViewer); err != nil {
|
||||
t.Fatalf("share: %v", err)
|
||||
}
|
||||
|
||||
// Upgrade to editor.
|
||||
up, err := s.UpdateShareRole(ctx, owner, g.ID, friend, domain.RoleEditor)
|
||||
if err != nil || up.Role != domain.RoleEditor {
|
||||
t.Fatalf("upgrade role = %+v, %v", up, err)
|
||||
}
|
||||
// The recipient can now mutate but still can't manage shares.
|
||||
bed := seedBed(t, s, owner, g.ID)
|
||||
nx := 400.0
|
||||
cur, _ := s.store.GetObject(ctx, bed.ID)
|
||||
if _, err := s.UpdateObject(ctx, friend, bed.ID, ObjectPatch{XCM: &nx}, cur.Version); err != nil {
|
||||
t.Errorf("editor should move an object: %v", err)
|
||||
}
|
||||
if _, err := s.ListShares(ctx, friend, g.ID); !errors.Is(err, domain.ErrForbidden) {
|
||||
t.Errorf("editor listing shares = %v, want ErrForbidden", err)
|
||||
}
|
||||
|
||||
// A recipient can remove themselves ("leave garden").
|
||||
if err := s.RemoveShare(ctx, friend, g.ID, friend); err != nil {
|
||||
t.Errorf("self-leave: %v", err)
|
||||
}
|
||||
// After leaving, the garden is invisible again.
|
||||
if _, err := s.GardenFull(ctx, friend, g.ID); !errors.Is(err, domain.ErrNotFound) {
|
||||
t.Errorf("after leaving, read = %v, want ErrNotFound", err)
|
||||
}
|
||||
// A non-participant can't remove someone else's share.
|
||||
stranger := seedUser(t, s, "[email protected]")
|
||||
if err := s.RemoveShare(ctx, stranger, g.ID, owner); !errors.Is(err, domain.ErrNotFound) {
|
||||
t.Errorf("stranger remove = %v, want ErrNotFound", err)
|
||||
}
|
||||
}
|
||||
@@ -23,7 +23,20 @@ func scanGarden(s scanner) (*domain.Garden, error) {
|
||||
return &g, nil
|
||||
}
|
||||
|
||||
// maxGardensListed caps ListGardensForOwner as a defensive backstop against a
|
||||
// scanGardenWithRole reads a garden row plus a trailing my_role column into the
|
||||
// computed Garden.MyRole field (used by the actor-scoped list).
|
||||
func scanGardenWithRole(s scanner) (*domain.Garden, error) {
|
||||
var g domain.Garden
|
||||
if err := s.Scan(
|
||||
&g.ID, &g.OwnerID, &g.Name, &g.WidthCM, &g.HeightCM,
|
||||
&g.UnitPref, &g.Notes, &g.Version, &g.CreatedAt, &g.UpdatedAt, &g.MyRole,
|
||||
); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return &g, nil
|
||||
}
|
||||
|
||||
// maxGardensListed caps the gardens list as a defensive backstop against a
|
||||
// pathologically large result. At household scale a user has a handful of
|
||||
// gardens, so this is never hit; genuine pagination is post-v1 if ever needed.
|
||||
const maxGardensListed = 1000
|
||||
@@ -56,13 +69,21 @@ func (d *DB) GetGarden(ctx context.Context, id int64) (*domain.Garden, error) {
|
||||
return g, nil
|
||||
}
|
||||
|
||||
// ListGardensForOwner returns the gardens owned by ownerID, newest first. The
|
||||
// slice is always non-nil (an empty list, not null). Shared-with-me gardens are
|
||||
// added in #16.
|
||||
func (d *DB) ListGardensForOwner(ctx context.Context, ownerID int64) ([]domain.Garden, error) {
|
||||
// ListGardensForActor returns the gardens an actor can see — those they own
|
||||
// (my_role "owner") plus those shared with them (my_role = the share's role) —
|
||||
// newest first. Always a non-nil slice.
|
||||
func (d *DB) ListGardensForActor(ctx context.Context, actorID int64) ([]domain.Garden, error) {
|
||||
rows, err := d.sql.QueryContext(ctx,
|
||||
`SELECT `+gardenColumns+` FROM gardens WHERE owner_id = ? ORDER BY created_at DESC, id DESC LIMIT ?`,
|
||||
ownerID, maxGardensListed,
|
||||
`SELECT `+gardenColumns+`, my_role FROM (
|
||||
SELECT `+gardenColumns+`, 'owner' AS my_role FROM gardens WHERE owner_id = ?
|
||||
|
gitea-actions
commented
🟡 SQL hardcodes 'owner' role string duplicating service-layer gardenRole.String() maintainability · flagged by 1 model
🪰 Gadfly · advisory 🟡 **SQL hardcodes 'owner' role string duplicating service-layer gardenRole.String()**
_maintainability · flagged by 1 model_
- `internal/store/gardens.go:78` — The `ListGardensForActor` query hardcodes the string literal `'owner'` for the owner role (`'owner' AS my_role`), while the service layer derives the same value via `roleOwner.String()`. This duplicates the role-string definition across store and service layers; if the API contract changes, both places must be updated or they drift apart.
<sub>🪰 Gadfly · advisory</sub>
|
||||
UNION ALL
|
||||
SELECT `+qualifyColumns("g", gardenColumns)+`, s.role AS my_role
|
||||
FROM gardens g JOIN garden_shares s ON s.garden_id = g.id
|
||||
WHERE s.user_id = ?
|
||||
)
|
||||
ORDER BY created_at DESC, id DESC
|
||||
LIMIT ?`,
|
||||
actorID, actorID, maxGardensListed,
|
||||
)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("store: list gardens: %w", err)
|
||||
@@ -71,7 +92,7 @@ func (d *DB) ListGardensForOwner(ctx context.Context, ownerID int64) ([]domain.G
|
||||
|
||||
gardens := []domain.Garden{}
|
||||
for rows.Next() {
|
||||
g, err := scanGarden(rows)
|
||||
g, err := scanGardenWithRole(rows)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("store: scan garden: %w", err)
|
||||
}
|
||||
|
||||
@@ -0,0 +1,134 @@
|
||||
package store
|
||||
|
||||
import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"errors"
|
||||
"fmt"
|
||||
|
||||
"gitea.stevedudenhoeffer.com/steve/pansy/internal/domain"
|
||||
)
|
||||
|
||||
// shareColumns lists garden_shares columns in the order scanShare expects.
|
||||
const shareColumns = `id, garden_id, user_id, role, created_by, version, created_at, updated_at`
|
||||
|
||||
func scanShare(s scanner) (*domain.GardenShare, error) {
|
||||
var sh domain.GardenShare
|
||||
if err := s.Scan(
|
||||
&sh.ID, &sh.GardenID, &sh.UserID, &sh.Role, &sh.CreatedBy,
|
||||
&sh.Version, &sh.CreatedAt, &sh.UpdatedAt,
|
||||
); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return &sh, nil
|
||||
}
|
||||
|
||||
// GetShareRole returns the role a user holds on a garden via a share row, and
|
||||
// whether such a row exists. Owner access is NOT a share row (it's implicit via
|
||||
// gardens.owner_id) — this only reports viewer/editor grants. It is the lookup
|
||||
// requireGardenRole uses to consult shares.
|
||||
func (d *DB) GetShareRole(ctx context.Context, gardenID, userID int64) (role string, found bool, err error) {
|
||||
err = d.sql.QueryRowContext(ctx,
|
||||
`SELECT role FROM garden_shares WHERE garden_id = ? AND user_id = ?`, gardenID, userID,
|
||||
).Scan(&role)
|
||||
if errors.Is(err, sql.ErrNoRows) {
|
||||
return "", false, nil
|
||||
}
|
||||
if err != nil {
|
||||
return "", false, fmt.Errorf("store: get share role: %w", err)
|
||||
}
|
||||
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
|
||||
|
||||
|
gitea-actions
commented
🟠 ListSharesForGarden is unbounded — missing LIMIT clause performance · flagged by 1 model
🪰 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
|
||||
// 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) {
|
||||
rows, err := d.sql.QueryContext(ctx,
|
||||
`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
|
||||
LIMIT ?`,
|
||||
gardenID, maxSharesListed,
|
||||
)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("store: list shares: %w", err)
|
||||
}
|
||||
defer rows.Close()
|
||||
|
gitea-actions
commented
🟡 ListSharesForGarden inlines the share-column scan, duplicating shareColumns order instead of extending scanShare maintainability · flagged by 1 model
🪰 Gadfly · advisory 🟡 **ListSharesForGarden inlines the share-column scan, duplicating shareColumns order instead of extending scanShare**
_maintainability · flagged by 1 model_
- **`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>
|
||||
|
||||
shares := []domain.ShareWithUser{}
|
||||
for rows.Next() {
|
||||
var sh domain.ShareWithUser
|
||||
if err := rows.Scan(
|
||||
&sh.ID, &sh.GardenID, &sh.UserID, &sh.Role, &sh.CreatedBy,
|
||||
&sh.Version, &sh.CreatedAt, &sh.UpdatedAt, &sh.Email, &sh.DisplayName,
|
||||
); err != nil {
|
||||
return nil, fmt.Errorf("store: scan share: %w", err)
|
||||
}
|
||||
shares = append(shares, sh)
|
||||
}
|
||||
if err := rows.Err(); err != nil {
|
||||
return nil, fmt.Errorf("store: iterate shares: %w", err)
|
||||
}
|
||||
return shares, nil
|
||||
}
|
||||
|
||||
// CreateShare inserts a share (fields validated by the service) and returns the
|
||||
// stored row. A duplicate (garden_id, user_id) trips the UNIQUE index and maps to
|
||||
// domain.ErrShareExists.
|
||||
func (d *DB) CreateShare(ctx context.Context, sh *domain.GardenShare) (*domain.GardenShare, error) {
|
||||
created, err := scanShare(d.sql.QueryRowContext(ctx,
|
||||
`INSERT INTO garden_shares (garden_id, user_id, role, created_by)
|
||||
VALUES (?, ?, ?, ?)
|
||||
RETURNING `+shareColumns,
|
||||
sh.GardenID, sh.UserID, sh.Role, sh.CreatedBy,
|
||||
))
|
||||
if err != nil {
|
||||
if isUniqueViolation(err) {
|
||||
return nil, domain.ErrShareExists
|
||||
}
|
||||
return nil, fmt.Errorf("store: insert share: %w", err)
|
||||
}
|
||||
return created, nil
|
||||
}
|
||||
|
||||
// UpdateShareRole changes a share's role, returning the updated row or
|
||||
// domain.ErrNotFound if no such share exists.
|
||||
func (d *DB) UpdateShareRole(ctx context.Context, gardenID, userID int64, role string) (*domain.GardenShare, error) {
|
||||
updated, err := scanShare(d.sql.QueryRowContext(ctx,
|
||||
`UPDATE garden_shares
|
||||
SET role = ?, version = version + 1,
|
||||
updated_at = strftime('%Y-%m-%dT%H:%M:%SZ', 'now')
|
||||
WHERE garden_id = ? AND user_id = ?
|
||||
RETURNING `+shareColumns,
|
||||
role, gardenID, userID,
|
||||
))
|
||||
if errors.Is(err, sql.ErrNoRows) {
|
||||
return nil, domain.ErrNotFound
|
||||
}
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("store: update share role: %w", err)
|
||||
}
|
||||
return updated, nil
|
||||
}
|
||||
|
||||
// DeleteShare removes a share. Returns domain.ErrNotFound if no row was deleted.
|
||||
func (d *DB) DeleteShare(ctx context.Context, gardenID, userID int64) error {
|
||||
res, err := d.sql.ExecContext(ctx,
|
||||
`DELETE FROM garden_shares WHERE garden_id = ? AND user_id = ?`, gardenID, userID)
|
||||
if err != nil {
|
||||
return fmt.Errorf("store: delete share: %w", err)
|
||||
}
|
||||
n, err := res.RowsAffected()
|
||||
if err != nil {
|
||||
return fmt.Errorf("store: share delete rows: %w", err)
|
||||
}
|
||||
if n == 0 {
|
||||
return domain.ErrNotFound
|
||||
}
|
||||
return nil
|
||||
}
|
||||
🟡 "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 ingardenRole.String()while the store SQL atinternal/store/gardens.go:78emits'owner' AS my_roleas a literal. There is nodomain.RoleOwnerconstant (onlyRoleViewer/RoleEditor). If either side drifts, owned-gardenMyRolesilently mismatches. Fix: adddomain.RoleOwner = "owner"and reference it in both places. -internal/store/shares.go:61—ListSharesForGardeninlines the share-column scan ins…🪰 Gadfly · advisory