From e79e2c6cf0bb8de6e221581d2643486bcbfd3deb Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Tue, 28 Apr 2026 16:39:14 +0000 Subject: [PATCH 01/12] fix(webhooks): enforce tenant dispatch scope Carry tenant_id through webhook dispatch jobs and re-check scope in the worker so tenant-scoped webhooks cannot receive another tenant's payload. Allow worker and embedding backfill commands to use an explicitly configured local DATABASE_URL while still rejecting implicit defaults. Refs ENG-796 --- AGENTS.md | 8 ++ cmd/backfill-embeddings/main.go | 2 +- cmd/worker/main.go | 2 +- internal/config/config.go | 49 +++++++ internal/config/config_test.go | 66 +++++++++ internal/observability/names.go | 1 + internal/repository/webhooks_repository.go | 55 +++---- .../repository/webhooks_repository_test.go | 126 ++++++++++++++++ internal/service/webhook_dispatch_args.go | 1 + internal/service/webhook_provider.go | 41 +++++- internal/service/webhook_provider_test.go | 134 +++++++++++++++++- internal/service/webhook_sender_test.go | 4 +- internal/service/webhook_tenant.go | 124 ++++++++++++++++ internal/service/webhook_tenant_test.go | 115 +++++++++++++++ internal/service/webhooks_service.go | 2 +- internal/service/webhooks_service_test.go | 4 +- internal/workers/webhook_dispatch.go | 30 ++++ internal/workers/webhook_dispatch_test.go | 96 ++++++++++++- 18 files changed, 810 insertions(+), 50 deletions(-) create mode 100644 internal/repository/webhooks_repository_test.go create mode 100644 internal/service/webhook_tenant.go create mode 100644 internal/service/webhook_tenant_test.go diff --git a/AGENTS.md b/AGENTS.md index f28cdc5..d8362ee 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,6 +25,14 @@ - Prefer Go naming conventions (CamelCase for exported, lowerCamel for unexported). - Keep package names short and domain-focused (e.g., `repository`, `service`). +## Multi-Tenancy & Data Isolation +- Treat `tenant_id` as a security boundary, not a convenience filter. Tenant-owned data must never be read, enqueued, dispatched, cached, searched, embedded, or deleted across tenants. +- When making a model, migration, API request, or repository change involving tenant-owned data, audit every downstream path that carries or derives from that data: handlers, services, repositories, message publishers, River job args, workers, webhook payloads, search, embeddings, bulk operations, logs, and metrics. +- Prefer tenant-aware repository/service methods for tenant-owned workflows. Avoid adding broad helpers that return all enabled/all matching resources when the caller is dispatching, processing, or exposing tenant data. +- Async jobs must carry the tenant boundary when the source data has one, and workers must re-check tenant scope before doing side effects. Do not rely only on enqueue-time filtering. +- Global resources may intentionally have `tenant_id = NULL`, but code must make that behavior explicit. A missing tenant on event data should not match tenant-scoped resources. +- For any tenant-scoping change, include verification at the boundary where the leak could happen: database query behavior, service fan-out, worker execution, and API behavior when relevant. Tests are the evidence; the invariant belongs in the architecture. + ## Testing Guidelines - Tests live under `tests/` and are run with `go test ./tests/...`. - Name test files `*_test.go` and test functions `TestXxx`. diff --git a/cmd/backfill-embeddings/main.go b/cmd/backfill-embeddings/main.go index 8554180..b522842 100644 --- a/cmd/backfill-embeddings/main.go +++ b/cmd/backfill-embeddings/main.go @@ -44,7 +44,7 @@ func run() int { return exitFailure } - if cfg.Database.URL == "" || cfg.Database.URL == config.DefaultDatabaseURL { + if cfg.Database.URL == "" || !config.DatabaseURLConfigured() { slog.Error("DATABASE_URL must be set explicitly for this binary (do not use the default test URL)") return exitFailure diff --git a/cmd/worker/main.go b/cmd/worker/main.go index 88b7eb8..5660320 100644 --- a/cmd/worker/main.go +++ b/cmd/worker/main.go @@ -32,7 +32,7 @@ func run() int { return exitFailure } - if cfg.Database.URL == "" || cfg.Database.URL == config.DefaultDatabaseURL { + if cfg.Database.URL == "" || !config.DatabaseURLConfigured() { slog.Error("DATABASE_URL must be set explicitly for hub-worker (do not use the default test URL)") return exitFailure diff --git a/internal/config/config.go b/internal/config/config.go index 2bd7cd5..8997304 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -204,6 +204,55 @@ func Load() (*Config, error) { return cfg, nil } +// DatabaseURLConfigured reports whether DATABASE_URL was explicitly set in the +// process environment or local .env file, rather than filled by Config defaults. +func DatabaseURLConfigured() bool { + return databaseURLConfigured(".env") +} + +func databaseURLConfigured(envFile string) bool { + if value, ok := os.LookupEnv("DATABASE_URL"); ok { + return strings.TrimSpace(value) != "" + } + + value, ok := lookupEnvFileValue(envFile, "DATABASE_URL") + if !ok { + return false + } + + return strings.TrimSpace(value) != "" +} + +func lookupEnvFileValue(path, key string) (string, bool) { + data, err := os.ReadFile(path) // #nosec G304 -- runtime reads fixed ".env"; tests pass temp files. + if err != nil { + return "", false + } + + for line := range strings.SplitSeq(string(data), "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + + line = strings.TrimSpace(strings.TrimPrefix(line, "export ")) + + name, value, ok := strings.Cut(line, "=") + if !ok || strings.TrimSpace(name) != key { + continue + } + + value = strings.TrimSpace(value) + if unquoted, unquoteErr := strconv.Unquote(value); unquoteErr == nil { + value = unquoted + } + + return value, true + } + + return "", false +} + // applyDefaults fills in default values for empty fields (cleanenv may leave nested struct defaults unset). func applyDefaults(cfg *Config) { if cfg.Server.Port == "" { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 48b4596..00f4449 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,6 +1,8 @@ package config import ( + "os" + "path/filepath" "testing" ) @@ -92,6 +94,70 @@ func TestLoadAlwaysReturnsNilError(t *testing.T) { } } +func TestDatabaseURLConfigured(t *testing.T) { + t.Run("true when environment variable is set to default local URL", func(t *testing.T) { + t.Setenv("DATABASE_URL", DefaultDatabaseURL) + + if !databaseURLConfigured(filepath.Join(t.TempDir(), ".env")) { + t.Fatal("databaseURLConfigured() = false, want true") + } + }) + + t.Run("false when environment variable is empty", func(t *testing.T) { + envFile := filepath.Join(t.TempDir(), ".env") + if err := os.WriteFile(envFile, []byte("DATABASE_URL=postgres://from-file\n"), 0o600); err != nil { + t.Fatalf("write env file: %v", err) + } + + t.Setenv("DATABASE_URL", "") + + if databaseURLConfigured(envFile) { + t.Fatal("databaseURLConfigured() = true, want false") + } + }) + + t.Run("true when env file has database URL", func(t *testing.T) { + restoreUnsetEnv(t, "DATABASE_URL") + + envFile := filepath.Join(t.TempDir(), ".env") + if err := os.WriteFile(envFile, []byte("DATABASE_URL=postgres://from-file\n"), 0o600); err != nil { + t.Fatalf("write env file: %v", err) + } + + if !databaseURLConfigured(envFile) { + t.Fatal("databaseURLConfigured() = false, want true") + } + }) + + t.Run("false when neither env nor env file has database URL", func(t *testing.T) { + restoreUnsetEnv(t, "DATABASE_URL") + + if databaseURLConfigured(filepath.Join(t.TempDir(), ".env")) { + t.Fatal("databaseURLConfigured() = true, want false") + } + }) +} + +//nolint:usetesting // This helper deliberately unsets env vars; t.Setenv cannot express an unset value. +func restoreUnsetEnv(t *testing.T, key string) { + t.Helper() + + oldValue, hadValue := os.LookupEnv(key) + if err := os.Unsetenv(key); err != nil { + t.Fatalf("unset %s: %v", key, err) + } + + t.Cleanup(func() { + if hadValue { + _ = os.Setenv(key, oldValue) + + return + } + + _ = os.Unsetenv(key) + }) +} + func TestLoad_WebhookDeliveryMaxAttempts(t *testing.T) { t.Run("default is 3 when unset", func(t *testing.T) { cfg, err := Load() diff --git a/internal/observability/names.go b/internal/observability/names.go index 794b26e..feecf27 100644 --- a/internal/observability/names.go +++ b/internal/observability/names.go @@ -63,6 +63,7 @@ var allowedDisabledReasons = map[string]bool{ // allowedDispatchReasons for hub_webhook_dispatch_errors_total. var allowedDispatchReasons = map[string]bool{ "get_webhook_failed": true, + "tenant_mismatch": true, } // allowedEmbeddingProviderReasons for hub_embedding_provider_errors_total. diff --git a/internal/repository/webhooks_repository.go b/internal/repository/webhooks_repository.go index 8509153..7e5f627 100644 --- a/internal/repository/webhooks_repository.go +++ b/internal/repository/webhooks_repository.go @@ -388,50 +388,35 @@ func (r *WebhooksRepository) ListEnabled(ctx context.Context) ([]models.Webhook, return webhooks, nil } -// ListEnabledForEventType retrieves all enabled webhooks that should receive a specific event type. -// Order is deterministic (ORDER BY id) so delivery behavior is consistent. -func (r *WebhooksRepository) ListEnabledForEventType(ctx context.Context, eventType string) ([]models.Webhook, error) { - query := ` +const listEnabledForEventTypeSelect = ` SELECT id, url, signing_key, enabled, tenant_id, created_at, updated_at, event_types, disabled_reason, disabled_at FROM webhooks WHERE enabled = true AND (event_types IS NULL OR event_types = '{}' OR event_types @> ARRAY[$1]::VARCHAR(64)[]) - ORDER BY id ` - rows, err := r.db.Query(ctx, query, eventType) - if err != nil { - return nil, fmt.Errorf("failed to list enabled webhooks for event type: %w", err) - } - defer rows.Close() - - webhooks := []models.Webhook{} - - for rows.Next() { - var ( - webhook models.Webhook - dbEventTypes []string - ) - - err := rows.Scan( - &webhook.ID, &webhook.URL, &webhook.SigningKey, &webhook.Enabled, - &webhook.TenantID, &webhook.CreatedAt, &webhook.UpdatedAt, &dbEventTypes, - &webhook.DisabledReason, &webhook.DisabledAt, - ) - if err != nil { - return nil, fmt.Errorf("failed to scan webhook: %w", err) - } - - webhook.EventTypes, err = parseDBEventTypes(dbEventTypes) - if err != nil { - return nil, err - } +// ListEnabledForEventTypeAndTenant retrieves enabled webhooks for an event type and tenant boundary. +// Global webhooks (tenant_id NULL) match every event. Tenant-scoped webhooks match only the same tenant. +// When tenantID is nil, only global webhooks are returned because scoped webhooks cannot be proven safe. +func (r *WebhooksRepository) ListEnabledForEventTypeAndTenant( + ctx context.Context, eventType string, tenantID *string, +) ([]models.Webhook, error) { + query := listEnabledForEventTypeSelect + args := []any{eventType} + + if tenantID == nil { + query += ` AND tenant_id IS NULL` + } else { + query += ` AND (tenant_id IS NULL OR tenant_id = $2)` - webhooks = append(webhooks, webhook) + args = append(args, *tenantID) } - if err := rows.Err(); err != nil { - return nil, fmt.Errorf("error iterating webhooks: %w", err) + query += ` ORDER BY id` + + webhooks, err := r.fetchWebhooks(ctx, query, args...) + if err != nil { + return nil, fmt.Errorf("list enabled webhooks for event type and tenant: %w", err) } return webhooks, nil diff --git a/internal/repository/webhooks_repository_test.go b/internal/repository/webhooks_repository_test.go new file mode 100644 index 0000000..b99d5cf --- /dev/null +++ b/internal/repository/webhooks_repository_test.go @@ -0,0 +1,126 @@ +package repository + +import ( + "context" + "os" + "strings" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/formbricks/hub/internal/config" + "github.com/formbricks/hub/internal/datatypes" + "github.com/formbricks/hub/internal/models" + "github.com/formbricks/hub/pkg/database" +) + +const repositoryTestDatabaseURL = "postgres://postgres:postgres@localhost:5432/test_db?sslmode=disable" + +func TestWebhooksRepository_ListEnabledForEventTypeAndTenant(t *testing.T) { + ctx := context.Background() + + databaseURL := os.Getenv("DATABASE_URL") + if databaseURL == "" { + databaseURL = repositoryTestDatabaseURL + } + + t.Setenv("DATABASE_URL", databaseURL) + + cfg, err := config.Load() + require.NoError(t, err) + db, err := database.NewPostgresPool(ctx, cfg.Database.URL, + database.WithPoolConfig(cfg.Database.PoolConfig()), + ) + require.NoError(t, err) + + defer db.Close() + + _, err = db.Exec(ctx, "DELETE FROM webhooks WHERE url LIKE 'https://tenant-scope.test/%'") + require.NoError(t, err) + + defer func() { + _, cleanupErr := db.Exec(ctx, "DELETE FROM webhooks WHERE url LIKE 'https://tenant-scope.test/%'") + require.NoError(t, cleanupErr) + }() + + repo := NewWebhooksRepository(db) + tenantA := "repo-scope-tenant-a" + tenantB := "repo-scope-tenant-b" + disabled := false + feedbackCreated := []datatypes.EventType{datatypes.FeedbackRecordCreated} + feedbackUpdated := []datatypes.EventType{datatypes.FeedbackRecordUpdated} + + globalWebhook := createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/global", nil, nil) + tenantAWebhook := createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/tenant-a", &tenantA, feedbackCreated) + tenantBWebhook := createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/tenant-b", &tenantB, feedbackCreated) + disabledTenantAWebhook := createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/disabled-a", &tenantA, feedbackCreated) + _, err = repo.Update(ctx, disabledTenantAWebhook.ID, &models.UpdateWebhookRequest{Enabled: &disabled}) + require.NoError(t, err) + createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/updated-only-a", &tenantA, feedbackUpdated) + + tenantAWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), &tenantA) + require.NoError(t, err) + assertRepositoryWebhookIDs(t, tenantAWebhooks, map[uuid.UUID]bool{ + globalWebhook.ID: true, + tenantAWebhook.ID: true, + }) + + tenantBWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), &tenantB) + require.NoError(t, err) + assertRepositoryWebhookIDs(t, tenantBWebhooks, map[uuid.UUID]bool{ + globalWebhook.ID: true, + tenantBWebhook.ID: true, + }) + + globalOnlyWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), nil) + require.NoError(t, err) + assertRepositoryWebhookIDs(t, globalOnlyWebhooks, map[uuid.UUID]bool{ + globalWebhook.ID: true, + }) +} + +func createWebhookForRepositoryTest( + ctx context.Context, + t *testing.T, + repo *WebhooksRepository, + url string, + tenantID *string, + eventTypes []datatypes.EventType, +) *models.Webhook { + t.Helper() + + webhook, err := repo.Create(ctx, &models.CreateWebhookRequest{ + URL: url, + SigningKey: "whsec_abcdefghijklmnopqrstuvwxyz123456", + TenantID: tenantID, + EventTypes: eventTypes, + }) + require.NoError(t, err) + + return webhook +} + +func assertRepositoryWebhookIDs(t *testing.T, webhooks []models.Webhook, wantIDs map[uuid.UUID]bool) { + t.Helper() + + gotIDs := make(map[uuid.UUID]bool, len(webhooks)) + for _, webhook := range webhooks { + if !strings.HasPrefix(webhook.URL, "https://tenant-scope.test/") { + continue + } + + if !wantIDs[webhook.ID] { + t.Fatalf("unexpected scoped test webhook returned: %+v", webhook) + } + + gotIDs[webhook.ID] = true + } + + assert.Len(t, gotIDs, len(wantIDs), "webhooks = %+v", webhooks) + + for id := range wantIDs { + assert.True(t, gotIDs[id], "missing webhook %s in %+v", id, webhooks) + } +} diff --git a/internal/service/webhook_dispatch_args.go b/internal/service/webhook_dispatch_args.go index 7c530d5..8d7715c 100644 --- a/internal/service/webhook_dispatch_args.go +++ b/internal/service/webhook_dispatch_args.go @@ -19,6 +19,7 @@ type WebhookDispatchArgs struct { Timestamp time.Time `json:"timestamp"` Data any `json:"data"` ChangedFields []string `json:"changed_fields,omitempty"` + TenantID *string `json:"tenant_id,omitempty"` WebhookID uuid.UUID `json:"webhook_id" river:"unique"` } diff --git a/internal/service/webhook_provider.go b/internal/service/webhook_provider.go index 3032771..810e490 100644 --- a/internal/service/webhook_provider.go +++ b/internal/service/webhook_provider.go @@ -10,6 +10,7 @@ import ( "github.com/riverqueue/river" "github.com/riverqueue/river/rivertype" + "github.com/formbricks/hub/internal/models" "github.com/formbricks/hub/internal/observability" ) @@ -52,10 +53,18 @@ func NewWebhookProvider( } } -// PublishEvent lists all enabled webhooks for the event type and enqueues one job per webhook, -// in batches of maxFanOut to avoid oversized InsertMany calls. +// PublishEvent lists enabled webhooks for the event type and tenant, then enqueues one job per webhook. +// Tenant-scoped webhooks are only eligible when the event payload has the same tenant_id. func (p *WebhookProvider) PublishEvent(ctx context.Context, event Event) { - webhooks, err := p.repo.ListEnabledForEventType(ctx, event.Type.String()) + tenantID := TenantIDPointerFromEventData(event.Data) + if tenantID == nil { + slog.Debug("webhook provider: event has no tenant_id; tenant-scoped webhooks are not eligible", + "event_id", event.ID, + "event_type", event.Type, + ) + } + + webhooks, err := p.repo.ListEnabledForEventTypeAndTenant(ctx, event.Type.String(), tenantID) if err != nil { if p.metrics != nil { p.metrics.RecordProviderError(ctx, "list_failed") @@ -70,6 +79,15 @@ func (p *WebhookProvider) PublishEvent(ctx context.Context, event Event) { return } + webhooks, skipped := filterWebhooksByTenant(webhooks, tenantID) + if skipped > 0 { + slog.Warn("webhook provider: skipped tenant-mismatched webhooks returned by repository", + "event_id", event.ID, + "event_type", event.Type, + "skipped", skipped, + ) + } + if len(webhooks) == 0 { return } @@ -83,7 +101,7 @@ func (p *WebhookProvider) PublishEvent(ctx context.Context, event Event) { ByPeriod: uniqueByPeriodHours * time.Hour, }, } - baseArgs := p.eventToArgs(event) + baseArgs := p.eventToArgs(event, tenantID) var enqueued int64 @@ -160,14 +178,27 @@ func (p *WebhookProvider) enqueueBackoffWithJitter(attempt int) time.Duration { return exp + jitter } +func filterWebhooksByTenant(webhooks []models.Webhook, tenantID *string) ([]models.Webhook, int) { + filtered := make([]models.Webhook, 0, len(webhooks)) + + for i := range webhooks { + if WebhookMatchesTenant(&webhooks[i], tenantID) { + filtered = append(filtered, webhooks[i]) + } + } + + return filtered, len(webhooks) - len(filtered) +} + // eventToArgs converts an Event to WebhookDispatchArgs (WebhookID must be set per webhook). -func (p *WebhookProvider) eventToArgs(event Event) WebhookDispatchArgs { +func (p *WebhookProvider) eventToArgs(event Event, tenantID *string) WebhookDispatchArgs { return WebhookDispatchArgs{ EventID: event.ID, EventType: event.Type.String(), Timestamp: event.Timestamp, Data: event.Data, ChangedFields: event.ChangedFields, + TenantID: tenantID, WebhookID: uuid.Nil, // set per webhook in PublishEvent } } diff --git a/internal/service/webhook_provider_test.go b/internal/service/webhook_provider_test.go index e536e2f..585b86a 100644 --- a/internal/service/webhook_provider_test.go +++ b/internal/service/webhook_provider_test.go @@ -37,13 +37,22 @@ func (m *mockWebhookInserter) InsertMany(_ context.Context, params []river.Inser return results, nil } -// mockProviderRepo implements only ListEnabledForEventType for provider tests. +// mockProviderRepo implements webhook listing for provider tests. type mockProviderRepo struct { - webhooks []models.Webhook - err error + webhooks []models.Webhook + err error + eventType string + tenantID *string + listCallCount int } -func (m *mockProviderRepo) ListEnabledForEventType(_ context.Context, _ string) ([]models.Webhook, error) { +func (m *mockProviderRepo) ListEnabledForEventTypeAndTenant( + _ context.Context, eventType string, tenantID *string, +) ([]models.Webhook, error) { + m.eventType = eventType + m.tenantID = cloneStringPointer(tenantID) + m.listCallCount++ + if m.err != nil { return nil, m.err } @@ -109,6 +118,14 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { provider.PublishEvent(ctx, event) + if repo.eventType != eventType.String() { + t.Errorf("eventType = %q, want %q", repo.eventType, eventType.String()) + } + + if repo.tenantID != nil { + t.Errorf("tenantID = %q, want nil for tenant-less event", *repo.tenantID) + } + if n := len(inserter.insertManyCalls); n != 1 { t.Fatalf("InsertMany called %d times, want 1", n) } @@ -137,6 +154,10 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { t.Errorf("param %d WebhookID = %v, want %v", i, args.WebhookID, wantID) } + if args.TenantID != nil { + t.Errorf("param %d TenantID = %q, want nil", i, *args.TenantID) + } + if p.InsertOpts == nil || p.InsertOpts.MaxAttempts != 3 { t.Errorf("param %d MaxAttempts = %v, want 3", i, p.InsertOpts) } @@ -216,4 +237,109 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { t.Errorf("second batch params length = %d, want 1", len(inserter.insertManyCalls[1])) } }) + + t.Run("filters tenant-scoped webhooks before enqueue", func(t *testing.T) { + tenantA := "org-123" + tenantB := "org-other" + inserter := &mockWebhookInserter{} + repo := &mockProviderRepo{ + webhooks: []models.Webhook{ + {ID: wh1}, + {ID: wh2, TenantID: &tenantA}, + {ID: uuid.Must(uuid.NewV7()), TenantID: &tenantB}, + }, + } + provider := NewWebhookProvider(inserter, repo, 3, 500, 0, 0, 0, nil) + event := Event{ + ID: eventID, + Type: eventType, + Timestamp: time.Now(), + Data: &models.FeedbackRecord{TenantID: tenantA}, + } + + provider.PublishEvent(ctx, event) + + if repo.tenantID == nil || *repo.tenantID != tenantA { + t.Fatalf("tenantID = %v, want %q", repo.tenantID, tenantA) + } + + if len(inserter.insertManyCalls) != 1 { + t.Fatalf("InsertMany called %d times, want 1", len(inserter.insertManyCalls)) + } + + params := inserter.insertManyCalls[0] + if len(params) != 2 { + t.Fatalf("InsertMany params length = %d, want 2", len(params)) + } + + gotWebhookIDs := map[uuid.UUID]bool{} + + for _, p := range params { + args, ok := p.Args.(WebhookDispatchArgs) + if !ok { + t.Fatalf("Args type = %T, want WebhookDispatchArgs", p.Args) + } + + if args.TenantID == nil || *args.TenantID != tenantA { + t.Errorf("TenantID = %v, want %q", args.TenantID, tenantA) + } + + gotWebhookIDs[args.WebhookID] = true + } + + if !gotWebhookIDs[wh1] || !gotWebhookIDs[wh2] { + t.Errorf("enqueued webhook IDs = %v, want global and matching tenant webhooks", gotWebhookIDs) + } + }) + + t.Run("tenant-less events only enqueue global webhooks", func(t *testing.T) { + tenantA := "org-123" + inserter := &mockWebhookInserter{} + repo := &mockProviderRepo{ + webhooks: []models.Webhook{ + {ID: wh1}, + {ID: wh2, TenantID: &tenantA}, + }, + } + provider := NewWebhookProvider(inserter, repo, 3, 500, 0, 0, 0, nil) + event := Event{ID: eventID, Type: eventType, Timestamp: time.Now(), Data: []uuid.UUID{uuid.Must(uuid.NewV7())}} + + provider.PublishEvent(ctx, event) + + if repo.tenantID != nil { + t.Fatalf("tenantID = %q, want nil", *repo.tenantID) + } + + if len(inserter.insertManyCalls) != 1 { + t.Fatalf("InsertMany called %d times, want 1", len(inserter.insertManyCalls)) + } + + params := inserter.insertManyCalls[0] + if len(params) != 1 { + t.Fatalf("InsertMany params length = %d, want 1", len(params)) + } + + args, ok := params[0].Args.(WebhookDispatchArgs) + if !ok { + t.Fatalf("Args type = %T, want WebhookDispatchArgs", params[0].Args) + } + + if args.WebhookID != wh1 { + t.Errorf("WebhookID = %v, want %v", args.WebhookID, wh1) + } + + if args.TenantID != nil { + t.Errorf("TenantID = %q, want nil", *args.TenantID) + } + }) +} + +func cloneStringPointer(value *string) *string { + if value == nil { + return nil + } + + v := *value + + return &v } diff --git a/internal/service/webhook_sender_test.go b/internal/service/webhook_sender_test.go index e1f6ea1..ee67974 100644 --- a/internal/service/webhook_sender_test.go +++ b/internal/service/webhook_sender_test.go @@ -54,7 +54,9 @@ func (m *mockSenderRepo) ListEnabled(_ context.Context) ([]models.Webhook, error return nil, nil } -func (m *mockSenderRepo) ListEnabledForEventType(_ context.Context, _ string) ([]models.Webhook, error) { +func (m *mockSenderRepo) ListEnabledForEventTypeAndTenant( + _ context.Context, _ string, _ *string, +) ([]models.Webhook, error) { return nil, nil } diff --git a/internal/service/webhook_tenant.go b/internal/service/webhook_tenant.go new file mode 100644 index 0000000..9ac21c2 --- /dev/null +++ b/internal/service/webhook_tenant.go @@ -0,0 +1,124 @@ +package service + +import ( + "encoding/json" + + "github.com/formbricks/hub/internal/models" +) + +// TenantIDFromEventData extracts tenant_id from known event payload shapes. +func TenantIDFromEventData(data any) (string, bool) { + switch payload := data.(type) { + case *models.FeedbackRecord: + if payload == nil { + return "", false + } + + return tenantIDFromString(payload.TenantID) + case models.FeedbackRecord: + return tenantIDFromString(payload.TenantID) + case *models.Webhook: + if payload == nil { + return "", false + } + + return tenantIDFromPointer(payload.TenantID) + case models.Webhook: + return tenantIDFromPointer(payload.TenantID) + case *models.WebhookPublic: + if payload == nil { + return "", false + } + + return tenantIDFromPointer(payload.TenantID) + case models.WebhookPublic: + return tenantIDFromPointer(payload.TenantID) + case map[string]any: + return tenantIDFromMapValue(payload["tenant_id"]) + case map[string]string: + return tenantIDFromString(payload["tenant_id"]) + case json.RawMessage: + return tenantIDFromRawJSON(payload) + } + + return tenantIDFromJSON(data) +} + +// TenantIDPointerFromEventData returns a detached pointer so it can be safely stored in job args. +func TenantIDPointerFromEventData(data any) *string { + tenantID, ok := TenantIDFromEventData(data) + if !ok { + return nil + } + + return stringPointer(tenantID) +} + +// WebhookMatchesTenant reports whether a webhook may receive an event with tenantID. +func WebhookMatchesTenant(webhook *models.Webhook, tenantID *string) bool { + if webhook == nil { + return false + } + + if webhook.TenantID == nil { + return true + } + + if tenantID == nil { + return false + } + + return *webhook.TenantID == *tenantID +} + +func tenantIDFromMapValue(value any) (string, bool) { + tenantID, ok := value.(string) + if !ok { + return "", false + } + + return tenantIDFromString(tenantID) +} + +func tenantIDFromPointer(tenantID *string) (string, bool) { + if tenantID == nil { + return "", false + } + + return tenantIDFromString(*tenantID) +} + +func tenantIDFromString(tenantID string) (string, bool) { + if tenantID == "" { + return "", false + } + + return tenantID, true +} + +func tenantIDFromJSON(data any) (string, bool) { + payload, err := json.Marshal(data) + if err != nil { + return "", false + } + + return tenantIDFromRawJSON(payload) +} + +func tenantIDFromRawJSON(payload []byte) (string, bool) { + var envelope struct { + TenantID *string `json:"tenant_id"` + } + + if err := json.Unmarshal(payload, &envelope); err != nil { + return "", false + } + + return tenantIDFromPointer(envelope.TenantID) +} + +func stringPointer(value string) *string { + v := value + + return &v +} diff --git a/internal/service/webhook_tenant_test.go b/internal/service/webhook_tenant_test.go new file mode 100644 index 0000000..8c3aa10 --- /dev/null +++ b/internal/service/webhook_tenant_test.go @@ -0,0 +1,115 @@ +package service + +import ( + "encoding/json" + "testing" + + "github.com/formbricks/hub/internal/models" +) + +func TestTenantIDFromEventData(t *testing.T) { + tenantID := "org-123" + + tests := []struct { + name string + data any + want string + ok bool + }{ + { + name: "feedback record pointer", + data: &models.FeedbackRecord{TenantID: tenantID}, + want: tenantID, + ok: true, + }, + { + name: "feedback record value", + data: models.FeedbackRecord{TenantID: tenantID}, + want: tenantID, + ok: true, + }, + { + name: "webhook pointer", + data: &models.Webhook{TenantID: &tenantID}, + want: tenantID, + ok: true, + }, + { + name: "map any", + data: map[string]any{"tenant_id": tenantID}, + want: tenantID, + ok: true, + }, + { + name: "map string", + data: map[string]string{"tenant_id": tenantID}, + want: tenantID, + ok: true, + }, + { + name: "raw json", + data: json.RawMessage(`{"tenant_id":"org-123"}`), + want: tenantID, + ok: true, + }, + { + name: "struct with json tag fallback", + data: struct { + TenantID string `json:"tenant_id"` + }{TenantID: tenantID}, + want: tenantID, + ok: true, + }, + { + name: "tenant-less data", + data: []string{"record-id"}, + ok: false, + }, + { + name: "empty tenant", + data: map[string]any{"tenant_id": ""}, + ok: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, ok := TenantIDFromEventData(tt.data) + if ok != tt.ok { + t.Fatalf("ok = %v, want %v", ok, tt.ok) + } + + if got != tt.want { + t.Errorf("tenantID = %q, want %q", got, tt.want) + } + }) + } +} + +func TestWebhookMatchesTenant(t *testing.T) { + tenantID := "org-123" + otherTenantID := "org-other" + + tests := []struct { + name string + webhook *models.Webhook + tenantID *string + want bool + }{ + {name: "global webhook matches tenant event", webhook: &models.Webhook{}, tenantID: &tenantID, want: true}, + {name: "global webhook matches tenant-less event", webhook: &models.Webhook{}, tenantID: nil, want: true}, + {name: "scoped webhook matches same tenant", webhook: &models.Webhook{TenantID: &tenantID}, tenantID: &tenantID, want: true}, + {name: "scoped webhook rejects different tenant", webhook: &models.Webhook{TenantID: &tenantID}, tenantID: &otherTenantID, want: false}, + {name: "scoped webhook rejects tenant-less event", webhook: &models.Webhook{TenantID: &tenantID}, tenantID: nil, want: false}, + {name: "nil webhook rejects", webhook: nil, tenantID: &tenantID, want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := WebhookMatchesTenant(tt.webhook, tt.tenantID) + if got != tt.want { + t.Errorf("WebhookMatchesTenant() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/internal/service/webhooks_service.go b/internal/service/webhooks_service.go index a6f4b89..b5c4d0a 100644 --- a/internal/service/webhooks_service.go +++ b/internal/service/webhooks_service.go @@ -33,7 +33,7 @@ type WebhooksRepository interface { Update(ctx context.Context, id uuid.UUID, req *models.UpdateWebhookRequest) (*models.Webhook, error) Delete(ctx context.Context, id uuid.UUID) error ListEnabled(ctx context.Context) ([]models.Webhook, error) - ListEnabledForEventType(ctx context.Context, eventType string) ([]models.Webhook, error) + ListEnabledForEventTypeAndTenant(ctx context.Context, eventType string, tenantID *string) ([]models.Webhook, error) } // WebhooksService handles business logic for webhooks. diff --git a/internal/service/webhooks_service_test.go b/internal/service/webhooks_service_test.go index 4ad8efc..b3addc2 100644 --- a/internal/service/webhooks_service_test.go +++ b/internal/service/webhooks_service_test.go @@ -52,7 +52,9 @@ func (m *mockWebhooksRepo) ListEnabled(_ context.Context) ([]models.Webhook, err return nil, nil } -func (m *mockWebhooksRepo) ListEnabledForEventType(_ context.Context, _ string) ([]models.Webhook, error) { +func (m *mockWebhooksRepo) ListEnabledForEventTypeAndTenant( + _ context.Context, _ string, _ *string, +) ([]models.Webhook, error) { return nil, nil } diff --git a/internal/workers/webhook_dispatch.go b/internal/workers/webhook_dispatch.go index 5c68a38..2e0f871 100644 --- a/internal/workers/webhook_dispatch.go +++ b/internal/workers/webhook_dispatch.go @@ -84,6 +84,36 @@ func (w *WebhookDispatchWorker) Work(ctx context.Context, job *river.Job[service return nil } + tenantID := args.TenantID + if tenantID == nil { + tenantID = service.TenantIDPointerFromEventData(args.Data) + } + + if !service.WebhookMatchesTenant(webhook, tenantID) { + if w.metrics != nil { + w.metrics.RecordDispatchError(ctx, "tenant_mismatch") + } + + var webhookTenantID any + if webhook.TenantID != nil { + webhookTenantID = *webhook.TenantID + } + + var eventTenantID any + if tenantID != nil { + eventTenantID = *tenantID + } + + slog.Error("webhook dispatch: tenant scope mismatch, skipping delivery", + "event_id", args.EventID, + "webhook_id", args.WebhookID, + "webhook_tenant_id", webhookTenantID, + "event_tenant_id", eventTenantID, + ) + + return nil + } + payload := argsToPayload(args) err = w.sender.Send(ctx, webhook, payload) diff --git a/internal/workers/webhook_dispatch_test.go b/internal/workers/webhook_dispatch_test.go index 750a842..91b1eff 100644 --- a/internal/workers/webhook_dispatch_test.go +++ b/internal/workers/webhook_dispatch_test.go @@ -31,10 +31,13 @@ func (m *mockDispatchRepo) Update(_ context.Context, _ uuid.UUID, req *models.Up } type mockSender struct { - err error + err error + calls int } func (m *mockSender) Send(_ context.Context, _ *models.Webhook, _ *service.WebhookPayload) error { + m.calls++ + return m.err } @@ -88,6 +91,97 @@ func TestWebhookDispatchWorker_Work(t *testing.T) { if repo.update != nil { t.Error("Update should not be called on success") } + + if sender.calls != 1 { + t.Errorf("Send called %d times, want 1", sender.calls) + } + }) + + t.Run("returns nil without send when scoped webhook tenant mismatches job tenant", func(t *testing.T) { + webhookTenant := "org-123" + eventTenant := "org-other" + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ + ID: webhookID, + Enabled: true, + URL: "http://x", + SigningKey: "sk", + TenantID: &webhookTenant, + }, + } + sender := &mockSender{} + worker := NewWebhookDispatchWorker(repo, sender, 15*time.Second, nil) + scopedArgs := args + scopedArgs.TenantID = &eventTenant + job := &river.Job[service.WebhookDispatchArgs]{JobRow: &rivertype.JobRow{}, Args: scopedArgs} + + err := worker.Work(ctx, job) + if err != nil { + t.Errorf("Work() error = %v, want nil", err) + } + + if sender.calls != 0 { + t.Errorf("Send called %d times, want 0", sender.calls) + } + + if repo.update != nil { + t.Error("Update should not be called for tenant mismatch") + } + }) + + t.Run("returns nil without send when legacy job data tenant mismatches scoped webhook", func(t *testing.T) { + webhookTenant := "org-123" + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ + ID: webhookID, + Enabled: true, + URL: "http://x", + SigningKey: "sk", + TenantID: &webhookTenant, + }, + } + sender := &mockSender{} + worker := NewWebhookDispatchWorker(repo, sender, 15*time.Second, nil) + scopedArgs := args + scopedArgs.Data = map[string]any{"tenant_id": "org-other"} + scopedArgs.TenantID = nil + job := &river.Job[service.WebhookDispatchArgs]{JobRow: &rivertype.JobRow{}, Args: scopedArgs} + + err := worker.Work(ctx, job) + if err != nil { + t.Errorf("Work() error = %v, want nil", err) + } + + if sender.calls != 0 { + t.Errorf("Send called %d times, want 0", sender.calls) + } + }) + + t.Run("sends when scoped webhook tenant matches job tenant", func(t *testing.T) { + tenantID := "org-123" + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ + ID: webhookID, + Enabled: true, + URL: "http://x", + SigningKey: "sk", + TenantID: &tenantID, + }, + } + sender := &mockSender{} + worker := NewWebhookDispatchWorker(repo, sender, 15*time.Second, nil) + scopedArgs := args + scopedArgs.TenantID = &tenantID + job := &river.Job[service.WebhookDispatchArgs]{JobRow: &rivertype.JobRow{}, Args: scopedArgs} + + err := worker.Work(ctx, job) + if err != nil { + t.Errorf("Work() error = %v, want nil", err) + } + + if sender.calls != 1 { + t.Errorf("Send called %d times, want 1", sender.calls) + } }) t.Run("returns error and does not update when send fails and attempt < max", func(t *testing.T) { From 6cba735963530f6accb1f1002032999802454181 Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Tue, 28 Apr 2026 16:39:14 +0000 Subject: [PATCH 02/12] fix(webhooks): enforce tenant dispatch scope Carry tenant_id through webhook dispatch jobs and re-check scope in the worker so tenant-scoped webhooks cannot receive another tenant's payload. Allow worker and embedding backfill commands to use an explicitly configured local DATABASE_URL while still rejecting implicit defaults. Refs ENG-796 --- AGENTS.md | 8 ++ internal/observability/names.go | 1 + internal/repository/webhooks_repository.go | 55 +++---- .../repository/webhooks_repository_test.go | 126 ++++++++++++++++ internal/service/webhook_dispatch_args.go | 1 + internal/service/webhook_provider.go | 41 +++++- internal/service/webhook_provider_test.go | 134 +++++++++++++++++- internal/service/webhook_sender_test.go | 4 +- internal/service/webhook_tenant.go | 124 ++++++++++++++++ internal/service/webhook_tenant_test.go | 115 +++++++++++++++ internal/service/webhooks_service.go | 2 +- internal/service/webhooks_service_test.go | 4 +- internal/workers/webhook_dispatch.go | 30 ++++ internal/workers/webhook_dispatch_test.go | 96 ++++++++++++- 14 files changed, 693 insertions(+), 48 deletions(-) create mode 100644 internal/repository/webhooks_repository_test.go create mode 100644 internal/service/webhook_tenant.go create mode 100644 internal/service/webhook_tenant_test.go diff --git a/AGENTS.md b/AGENTS.md index f28cdc5..d8362ee 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,6 +25,14 @@ - Prefer Go naming conventions (CamelCase for exported, lowerCamel for unexported). - Keep package names short and domain-focused (e.g., `repository`, `service`). +## Multi-Tenancy & Data Isolation +- Treat `tenant_id` as a security boundary, not a convenience filter. Tenant-owned data must never be read, enqueued, dispatched, cached, searched, embedded, or deleted across tenants. +- When making a model, migration, API request, or repository change involving tenant-owned data, audit every downstream path that carries or derives from that data: handlers, services, repositories, message publishers, River job args, workers, webhook payloads, search, embeddings, bulk operations, logs, and metrics. +- Prefer tenant-aware repository/service methods for tenant-owned workflows. Avoid adding broad helpers that return all enabled/all matching resources when the caller is dispatching, processing, or exposing tenant data. +- Async jobs must carry the tenant boundary when the source data has one, and workers must re-check tenant scope before doing side effects. Do not rely only on enqueue-time filtering. +- Global resources may intentionally have `tenant_id = NULL`, but code must make that behavior explicit. A missing tenant on event data should not match tenant-scoped resources. +- For any tenant-scoping change, include verification at the boundary where the leak could happen: database query behavior, service fan-out, worker execution, and API behavior when relevant. Tests are the evidence; the invariant belongs in the architecture. + ## Testing Guidelines - Tests live under `tests/` and are run with `go test ./tests/...`. - Name test files `*_test.go` and test functions `TestXxx`. diff --git a/internal/observability/names.go b/internal/observability/names.go index 794b26e..feecf27 100644 --- a/internal/observability/names.go +++ b/internal/observability/names.go @@ -63,6 +63,7 @@ var allowedDisabledReasons = map[string]bool{ // allowedDispatchReasons for hub_webhook_dispatch_errors_total. var allowedDispatchReasons = map[string]bool{ "get_webhook_failed": true, + "tenant_mismatch": true, } // allowedEmbeddingProviderReasons for hub_embedding_provider_errors_total. diff --git a/internal/repository/webhooks_repository.go b/internal/repository/webhooks_repository.go index 8509153..7e5f627 100644 --- a/internal/repository/webhooks_repository.go +++ b/internal/repository/webhooks_repository.go @@ -388,50 +388,35 @@ func (r *WebhooksRepository) ListEnabled(ctx context.Context) ([]models.Webhook, return webhooks, nil } -// ListEnabledForEventType retrieves all enabled webhooks that should receive a specific event type. -// Order is deterministic (ORDER BY id) so delivery behavior is consistent. -func (r *WebhooksRepository) ListEnabledForEventType(ctx context.Context, eventType string) ([]models.Webhook, error) { - query := ` +const listEnabledForEventTypeSelect = ` SELECT id, url, signing_key, enabled, tenant_id, created_at, updated_at, event_types, disabled_reason, disabled_at FROM webhooks WHERE enabled = true AND (event_types IS NULL OR event_types = '{}' OR event_types @> ARRAY[$1]::VARCHAR(64)[]) - ORDER BY id ` - rows, err := r.db.Query(ctx, query, eventType) - if err != nil { - return nil, fmt.Errorf("failed to list enabled webhooks for event type: %w", err) - } - defer rows.Close() - - webhooks := []models.Webhook{} - - for rows.Next() { - var ( - webhook models.Webhook - dbEventTypes []string - ) - - err := rows.Scan( - &webhook.ID, &webhook.URL, &webhook.SigningKey, &webhook.Enabled, - &webhook.TenantID, &webhook.CreatedAt, &webhook.UpdatedAt, &dbEventTypes, - &webhook.DisabledReason, &webhook.DisabledAt, - ) - if err != nil { - return nil, fmt.Errorf("failed to scan webhook: %w", err) - } - - webhook.EventTypes, err = parseDBEventTypes(dbEventTypes) - if err != nil { - return nil, err - } +// ListEnabledForEventTypeAndTenant retrieves enabled webhooks for an event type and tenant boundary. +// Global webhooks (tenant_id NULL) match every event. Tenant-scoped webhooks match only the same tenant. +// When tenantID is nil, only global webhooks are returned because scoped webhooks cannot be proven safe. +func (r *WebhooksRepository) ListEnabledForEventTypeAndTenant( + ctx context.Context, eventType string, tenantID *string, +) ([]models.Webhook, error) { + query := listEnabledForEventTypeSelect + args := []any{eventType} + + if tenantID == nil { + query += ` AND tenant_id IS NULL` + } else { + query += ` AND (tenant_id IS NULL OR tenant_id = $2)` - webhooks = append(webhooks, webhook) + args = append(args, *tenantID) } - if err := rows.Err(); err != nil { - return nil, fmt.Errorf("error iterating webhooks: %w", err) + query += ` ORDER BY id` + + webhooks, err := r.fetchWebhooks(ctx, query, args...) + if err != nil { + return nil, fmt.Errorf("list enabled webhooks for event type and tenant: %w", err) } return webhooks, nil diff --git a/internal/repository/webhooks_repository_test.go b/internal/repository/webhooks_repository_test.go new file mode 100644 index 0000000..b99d5cf --- /dev/null +++ b/internal/repository/webhooks_repository_test.go @@ -0,0 +1,126 @@ +package repository + +import ( + "context" + "os" + "strings" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/formbricks/hub/internal/config" + "github.com/formbricks/hub/internal/datatypes" + "github.com/formbricks/hub/internal/models" + "github.com/formbricks/hub/pkg/database" +) + +const repositoryTestDatabaseURL = "postgres://postgres:postgres@localhost:5432/test_db?sslmode=disable" + +func TestWebhooksRepository_ListEnabledForEventTypeAndTenant(t *testing.T) { + ctx := context.Background() + + databaseURL := os.Getenv("DATABASE_URL") + if databaseURL == "" { + databaseURL = repositoryTestDatabaseURL + } + + t.Setenv("DATABASE_URL", databaseURL) + + cfg, err := config.Load() + require.NoError(t, err) + db, err := database.NewPostgresPool(ctx, cfg.Database.URL, + database.WithPoolConfig(cfg.Database.PoolConfig()), + ) + require.NoError(t, err) + + defer db.Close() + + _, err = db.Exec(ctx, "DELETE FROM webhooks WHERE url LIKE 'https://tenant-scope.test/%'") + require.NoError(t, err) + + defer func() { + _, cleanupErr := db.Exec(ctx, "DELETE FROM webhooks WHERE url LIKE 'https://tenant-scope.test/%'") + require.NoError(t, cleanupErr) + }() + + repo := NewWebhooksRepository(db) + tenantA := "repo-scope-tenant-a" + tenantB := "repo-scope-tenant-b" + disabled := false + feedbackCreated := []datatypes.EventType{datatypes.FeedbackRecordCreated} + feedbackUpdated := []datatypes.EventType{datatypes.FeedbackRecordUpdated} + + globalWebhook := createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/global", nil, nil) + tenantAWebhook := createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/tenant-a", &tenantA, feedbackCreated) + tenantBWebhook := createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/tenant-b", &tenantB, feedbackCreated) + disabledTenantAWebhook := createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/disabled-a", &tenantA, feedbackCreated) + _, err = repo.Update(ctx, disabledTenantAWebhook.ID, &models.UpdateWebhookRequest{Enabled: &disabled}) + require.NoError(t, err) + createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/updated-only-a", &tenantA, feedbackUpdated) + + tenantAWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), &tenantA) + require.NoError(t, err) + assertRepositoryWebhookIDs(t, tenantAWebhooks, map[uuid.UUID]bool{ + globalWebhook.ID: true, + tenantAWebhook.ID: true, + }) + + tenantBWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), &tenantB) + require.NoError(t, err) + assertRepositoryWebhookIDs(t, tenantBWebhooks, map[uuid.UUID]bool{ + globalWebhook.ID: true, + tenantBWebhook.ID: true, + }) + + globalOnlyWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), nil) + require.NoError(t, err) + assertRepositoryWebhookIDs(t, globalOnlyWebhooks, map[uuid.UUID]bool{ + globalWebhook.ID: true, + }) +} + +func createWebhookForRepositoryTest( + ctx context.Context, + t *testing.T, + repo *WebhooksRepository, + url string, + tenantID *string, + eventTypes []datatypes.EventType, +) *models.Webhook { + t.Helper() + + webhook, err := repo.Create(ctx, &models.CreateWebhookRequest{ + URL: url, + SigningKey: "whsec_abcdefghijklmnopqrstuvwxyz123456", + TenantID: tenantID, + EventTypes: eventTypes, + }) + require.NoError(t, err) + + return webhook +} + +func assertRepositoryWebhookIDs(t *testing.T, webhooks []models.Webhook, wantIDs map[uuid.UUID]bool) { + t.Helper() + + gotIDs := make(map[uuid.UUID]bool, len(webhooks)) + for _, webhook := range webhooks { + if !strings.HasPrefix(webhook.URL, "https://tenant-scope.test/") { + continue + } + + if !wantIDs[webhook.ID] { + t.Fatalf("unexpected scoped test webhook returned: %+v", webhook) + } + + gotIDs[webhook.ID] = true + } + + assert.Len(t, gotIDs, len(wantIDs), "webhooks = %+v", webhooks) + + for id := range wantIDs { + assert.True(t, gotIDs[id], "missing webhook %s in %+v", id, webhooks) + } +} diff --git a/internal/service/webhook_dispatch_args.go b/internal/service/webhook_dispatch_args.go index 7c530d5..8d7715c 100644 --- a/internal/service/webhook_dispatch_args.go +++ b/internal/service/webhook_dispatch_args.go @@ -19,6 +19,7 @@ type WebhookDispatchArgs struct { Timestamp time.Time `json:"timestamp"` Data any `json:"data"` ChangedFields []string `json:"changed_fields,omitempty"` + TenantID *string `json:"tenant_id,omitempty"` WebhookID uuid.UUID `json:"webhook_id" river:"unique"` } diff --git a/internal/service/webhook_provider.go b/internal/service/webhook_provider.go index 3032771..810e490 100644 --- a/internal/service/webhook_provider.go +++ b/internal/service/webhook_provider.go @@ -10,6 +10,7 @@ import ( "github.com/riverqueue/river" "github.com/riverqueue/river/rivertype" + "github.com/formbricks/hub/internal/models" "github.com/formbricks/hub/internal/observability" ) @@ -52,10 +53,18 @@ func NewWebhookProvider( } } -// PublishEvent lists all enabled webhooks for the event type and enqueues one job per webhook, -// in batches of maxFanOut to avoid oversized InsertMany calls. +// PublishEvent lists enabled webhooks for the event type and tenant, then enqueues one job per webhook. +// Tenant-scoped webhooks are only eligible when the event payload has the same tenant_id. func (p *WebhookProvider) PublishEvent(ctx context.Context, event Event) { - webhooks, err := p.repo.ListEnabledForEventType(ctx, event.Type.String()) + tenantID := TenantIDPointerFromEventData(event.Data) + if tenantID == nil { + slog.Debug("webhook provider: event has no tenant_id; tenant-scoped webhooks are not eligible", + "event_id", event.ID, + "event_type", event.Type, + ) + } + + webhooks, err := p.repo.ListEnabledForEventTypeAndTenant(ctx, event.Type.String(), tenantID) if err != nil { if p.metrics != nil { p.metrics.RecordProviderError(ctx, "list_failed") @@ -70,6 +79,15 @@ func (p *WebhookProvider) PublishEvent(ctx context.Context, event Event) { return } + webhooks, skipped := filterWebhooksByTenant(webhooks, tenantID) + if skipped > 0 { + slog.Warn("webhook provider: skipped tenant-mismatched webhooks returned by repository", + "event_id", event.ID, + "event_type", event.Type, + "skipped", skipped, + ) + } + if len(webhooks) == 0 { return } @@ -83,7 +101,7 @@ func (p *WebhookProvider) PublishEvent(ctx context.Context, event Event) { ByPeriod: uniqueByPeriodHours * time.Hour, }, } - baseArgs := p.eventToArgs(event) + baseArgs := p.eventToArgs(event, tenantID) var enqueued int64 @@ -160,14 +178,27 @@ func (p *WebhookProvider) enqueueBackoffWithJitter(attempt int) time.Duration { return exp + jitter } +func filterWebhooksByTenant(webhooks []models.Webhook, tenantID *string) ([]models.Webhook, int) { + filtered := make([]models.Webhook, 0, len(webhooks)) + + for i := range webhooks { + if WebhookMatchesTenant(&webhooks[i], tenantID) { + filtered = append(filtered, webhooks[i]) + } + } + + return filtered, len(webhooks) - len(filtered) +} + // eventToArgs converts an Event to WebhookDispatchArgs (WebhookID must be set per webhook). -func (p *WebhookProvider) eventToArgs(event Event) WebhookDispatchArgs { +func (p *WebhookProvider) eventToArgs(event Event, tenantID *string) WebhookDispatchArgs { return WebhookDispatchArgs{ EventID: event.ID, EventType: event.Type.String(), Timestamp: event.Timestamp, Data: event.Data, ChangedFields: event.ChangedFields, + TenantID: tenantID, WebhookID: uuid.Nil, // set per webhook in PublishEvent } } diff --git a/internal/service/webhook_provider_test.go b/internal/service/webhook_provider_test.go index e536e2f..585b86a 100644 --- a/internal/service/webhook_provider_test.go +++ b/internal/service/webhook_provider_test.go @@ -37,13 +37,22 @@ func (m *mockWebhookInserter) InsertMany(_ context.Context, params []river.Inser return results, nil } -// mockProviderRepo implements only ListEnabledForEventType for provider tests. +// mockProviderRepo implements webhook listing for provider tests. type mockProviderRepo struct { - webhooks []models.Webhook - err error + webhooks []models.Webhook + err error + eventType string + tenantID *string + listCallCount int } -func (m *mockProviderRepo) ListEnabledForEventType(_ context.Context, _ string) ([]models.Webhook, error) { +func (m *mockProviderRepo) ListEnabledForEventTypeAndTenant( + _ context.Context, eventType string, tenantID *string, +) ([]models.Webhook, error) { + m.eventType = eventType + m.tenantID = cloneStringPointer(tenantID) + m.listCallCount++ + if m.err != nil { return nil, m.err } @@ -109,6 +118,14 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { provider.PublishEvent(ctx, event) + if repo.eventType != eventType.String() { + t.Errorf("eventType = %q, want %q", repo.eventType, eventType.String()) + } + + if repo.tenantID != nil { + t.Errorf("tenantID = %q, want nil for tenant-less event", *repo.tenantID) + } + if n := len(inserter.insertManyCalls); n != 1 { t.Fatalf("InsertMany called %d times, want 1", n) } @@ -137,6 +154,10 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { t.Errorf("param %d WebhookID = %v, want %v", i, args.WebhookID, wantID) } + if args.TenantID != nil { + t.Errorf("param %d TenantID = %q, want nil", i, *args.TenantID) + } + if p.InsertOpts == nil || p.InsertOpts.MaxAttempts != 3 { t.Errorf("param %d MaxAttempts = %v, want 3", i, p.InsertOpts) } @@ -216,4 +237,109 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { t.Errorf("second batch params length = %d, want 1", len(inserter.insertManyCalls[1])) } }) + + t.Run("filters tenant-scoped webhooks before enqueue", func(t *testing.T) { + tenantA := "org-123" + tenantB := "org-other" + inserter := &mockWebhookInserter{} + repo := &mockProviderRepo{ + webhooks: []models.Webhook{ + {ID: wh1}, + {ID: wh2, TenantID: &tenantA}, + {ID: uuid.Must(uuid.NewV7()), TenantID: &tenantB}, + }, + } + provider := NewWebhookProvider(inserter, repo, 3, 500, 0, 0, 0, nil) + event := Event{ + ID: eventID, + Type: eventType, + Timestamp: time.Now(), + Data: &models.FeedbackRecord{TenantID: tenantA}, + } + + provider.PublishEvent(ctx, event) + + if repo.tenantID == nil || *repo.tenantID != tenantA { + t.Fatalf("tenantID = %v, want %q", repo.tenantID, tenantA) + } + + if len(inserter.insertManyCalls) != 1 { + t.Fatalf("InsertMany called %d times, want 1", len(inserter.insertManyCalls)) + } + + params := inserter.insertManyCalls[0] + if len(params) != 2 { + t.Fatalf("InsertMany params length = %d, want 2", len(params)) + } + + gotWebhookIDs := map[uuid.UUID]bool{} + + for _, p := range params { + args, ok := p.Args.(WebhookDispatchArgs) + if !ok { + t.Fatalf("Args type = %T, want WebhookDispatchArgs", p.Args) + } + + if args.TenantID == nil || *args.TenantID != tenantA { + t.Errorf("TenantID = %v, want %q", args.TenantID, tenantA) + } + + gotWebhookIDs[args.WebhookID] = true + } + + if !gotWebhookIDs[wh1] || !gotWebhookIDs[wh2] { + t.Errorf("enqueued webhook IDs = %v, want global and matching tenant webhooks", gotWebhookIDs) + } + }) + + t.Run("tenant-less events only enqueue global webhooks", func(t *testing.T) { + tenantA := "org-123" + inserter := &mockWebhookInserter{} + repo := &mockProviderRepo{ + webhooks: []models.Webhook{ + {ID: wh1}, + {ID: wh2, TenantID: &tenantA}, + }, + } + provider := NewWebhookProvider(inserter, repo, 3, 500, 0, 0, 0, nil) + event := Event{ID: eventID, Type: eventType, Timestamp: time.Now(), Data: []uuid.UUID{uuid.Must(uuid.NewV7())}} + + provider.PublishEvent(ctx, event) + + if repo.tenantID != nil { + t.Fatalf("tenantID = %q, want nil", *repo.tenantID) + } + + if len(inserter.insertManyCalls) != 1 { + t.Fatalf("InsertMany called %d times, want 1", len(inserter.insertManyCalls)) + } + + params := inserter.insertManyCalls[0] + if len(params) != 1 { + t.Fatalf("InsertMany params length = %d, want 1", len(params)) + } + + args, ok := params[0].Args.(WebhookDispatchArgs) + if !ok { + t.Fatalf("Args type = %T, want WebhookDispatchArgs", params[0].Args) + } + + if args.WebhookID != wh1 { + t.Errorf("WebhookID = %v, want %v", args.WebhookID, wh1) + } + + if args.TenantID != nil { + t.Errorf("TenantID = %q, want nil", *args.TenantID) + } + }) +} + +func cloneStringPointer(value *string) *string { + if value == nil { + return nil + } + + v := *value + + return &v } diff --git a/internal/service/webhook_sender_test.go b/internal/service/webhook_sender_test.go index e1f6ea1..ee67974 100644 --- a/internal/service/webhook_sender_test.go +++ b/internal/service/webhook_sender_test.go @@ -54,7 +54,9 @@ func (m *mockSenderRepo) ListEnabled(_ context.Context) ([]models.Webhook, error return nil, nil } -func (m *mockSenderRepo) ListEnabledForEventType(_ context.Context, _ string) ([]models.Webhook, error) { +func (m *mockSenderRepo) ListEnabledForEventTypeAndTenant( + _ context.Context, _ string, _ *string, +) ([]models.Webhook, error) { return nil, nil } diff --git a/internal/service/webhook_tenant.go b/internal/service/webhook_tenant.go new file mode 100644 index 0000000..9ac21c2 --- /dev/null +++ b/internal/service/webhook_tenant.go @@ -0,0 +1,124 @@ +package service + +import ( + "encoding/json" + + "github.com/formbricks/hub/internal/models" +) + +// TenantIDFromEventData extracts tenant_id from known event payload shapes. +func TenantIDFromEventData(data any) (string, bool) { + switch payload := data.(type) { + case *models.FeedbackRecord: + if payload == nil { + return "", false + } + + return tenantIDFromString(payload.TenantID) + case models.FeedbackRecord: + return tenantIDFromString(payload.TenantID) + case *models.Webhook: + if payload == nil { + return "", false + } + + return tenantIDFromPointer(payload.TenantID) + case models.Webhook: + return tenantIDFromPointer(payload.TenantID) + case *models.WebhookPublic: + if payload == nil { + return "", false + } + + return tenantIDFromPointer(payload.TenantID) + case models.WebhookPublic: + return tenantIDFromPointer(payload.TenantID) + case map[string]any: + return tenantIDFromMapValue(payload["tenant_id"]) + case map[string]string: + return tenantIDFromString(payload["tenant_id"]) + case json.RawMessage: + return tenantIDFromRawJSON(payload) + } + + return tenantIDFromJSON(data) +} + +// TenantIDPointerFromEventData returns a detached pointer so it can be safely stored in job args. +func TenantIDPointerFromEventData(data any) *string { + tenantID, ok := TenantIDFromEventData(data) + if !ok { + return nil + } + + return stringPointer(tenantID) +} + +// WebhookMatchesTenant reports whether a webhook may receive an event with tenantID. +func WebhookMatchesTenant(webhook *models.Webhook, tenantID *string) bool { + if webhook == nil { + return false + } + + if webhook.TenantID == nil { + return true + } + + if tenantID == nil { + return false + } + + return *webhook.TenantID == *tenantID +} + +func tenantIDFromMapValue(value any) (string, bool) { + tenantID, ok := value.(string) + if !ok { + return "", false + } + + return tenantIDFromString(tenantID) +} + +func tenantIDFromPointer(tenantID *string) (string, bool) { + if tenantID == nil { + return "", false + } + + return tenantIDFromString(*tenantID) +} + +func tenantIDFromString(tenantID string) (string, bool) { + if tenantID == "" { + return "", false + } + + return tenantID, true +} + +func tenantIDFromJSON(data any) (string, bool) { + payload, err := json.Marshal(data) + if err != nil { + return "", false + } + + return tenantIDFromRawJSON(payload) +} + +func tenantIDFromRawJSON(payload []byte) (string, bool) { + var envelope struct { + TenantID *string `json:"tenant_id"` + } + + if err := json.Unmarshal(payload, &envelope); err != nil { + return "", false + } + + return tenantIDFromPointer(envelope.TenantID) +} + +func stringPointer(value string) *string { + v := value + + return &v +} diff --git a/internal/service/webhook_tenant_test.go b/internal/service/webhook_tenant_test.go new file mode 100644 index 0000000..8c3aa10 --- /dev/null +++ b/internal/service/webhook_tenant_test.go @@ -0,0 +1,115 @@ +package service + +import ( + "encoding/json" + "testing" + + "github.com/formbricks/hub/internal/models" +) + +func TestTenantIDFromEventData(t *testing.T) { + tenantID := "org-123" + + tests := []struct { + name string + data any + want string + ok bool + }{ + { + name: "feedback record pointer", + data: &models.FeedbackRecord{TenantID: tenantID}, + want: tenantID, + ok: true, + }, + { + name: "feedback record value", + data: models.FeedbackRecord{TenantID: tenantID}, + want: tenantID, + ok: true, + }, + { + name: "webhook pointer", + data: &models.Webhook{TenantID: &tenantID}, + want: tenantID, + ok: true, + }, + { + name: "map any", + data: map[string]any{"tenant_id": tenantID}, + want: tenantID, + ok: true, + }, + { + name: "map string", + data: map[string]string{"tenant_id": tenantID}, + want: tenantID, + ok: true, + }, + { + name: "raw json", + data: json.RawMessage(`{"tenant_id":"org-123"}`), + want: tenantID, + ok: true, + }, + { + name: "struct with json tag fallback", + data: struct { + TenantID string `json:"tenant_id"` + }{TenantID: tenantID}, + want: tenantID, + ok: true, + }, + { + name: "tenant-less data", + data: []string{"record-id"}, + ok: false, + }, + { + name: "empty tenant", + data: map[string]any{"tenant_id": ""}, + ok: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, ok := TenantIDFromEventData(tt.data) + if ok != tt.ok { + t.Fatalf("ok = %v, want %v", ok, tt.ok) + } + + if got != tt.want { + t.Errorf("tenantID = %q, want %q", got, tt.want) + } + }) + } +} + +func TestWebhookMatchesTenant(t *testing.T) { + tenantID := "org-123" + otherTenantID := "org-other" + + tests := []struct { + name string + webhook *models.Webhook + tenantID *string + want bool + }{ + {name: "global webhook matches tenant event", webhook: &models.Webhook{}, tenantID: &tenantID, want: true}, + {name: "global webhook matches tenant-less event", webhook: &models.Webhook{}, tenantID: nil, want: true}, + {name: "scoped webhook matches same tenant", webhook: &models.Webhook{TenantID: &tenantID}, tenantID: &tenantID, want: true}, + {name: "scoped webhook rejects different tenant", webhook: &models.Webhook{TenantID: &tenantID}, tenantID: &otherTenantID, want: false}, + {name: "scoped webhook rejects tenant-less event", webhook: &models.Webhook{TenantID: &tenantID}, tenantID: nil, want: false}, + {name: "nil webhook rejects", webhook: nil, tenantID: &tenantID, want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := WebhookMatchesTenant(tt.webhook, tt.tenantID) + if got != tt.want { + t.Errorf("WebhookMatchesTenant() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/internal/service/webhooks_service.go b/internal/service/webhooks_service.go index a6f4b89..b5c4d0a 100644 --- a/internal/service/webhooks_service.go +++ b/internal/service/webhooks_service.go @@ -33,7 +33,7 @@ type WebhooksRepository interface { Update(ctx context.Context, id uuid.UUID, req *models.UpdateWebhookRequest) (*models.Webhook, error) Delete(ctx context.Context, id uuid.UUID) error ListEnabled(ctx context.Context) ([]models.Webhook, error) - ListEnabledForEventType(ctx context.Context, eventType string) ([]models.Webhook, error) + ListEnabledForEventTypeAndTenant(ctx context.Context, eventType string, tenantID *string) ([]models.Webhook, error) } // WebhooksService handles business logic for webhooks. diff --git a/internal/service/webhooks_service_test.go b/internal/service/webhooks_service_test.go index 4ad8efc..b3addc2 100644 --- a/internal/service/webhooks_service_test.go +++ b/internal/service/webhooks_service_test.go @@ -52,7 +52,9 @@ func (m *mockWebhooksRepo) ListEnabled(_ context.Context) ([]models.Webhook, err return nil, nil } -func (m *mockWebhooksRepo) ListEnabledForEventType(_ context.Context, _ string) ([]models.Webhook, error) { +func (m *mockWebhooksRepo) ListEnabledForEventTypeAndTenant( + _ context.Context, _ string, _ *string, +) ([]models.Webhook, error) { return nil, nil } diff --git a/internal/workers/webhook_dispatch.go b/internal/workers/webhook_dispatch.go index 5c68a38..2e0f871 100644 --- a/internal/workers/webhook_dispatch.go +++ b/internal/workers/webhook_dispatch.go @@ -84,6 +84,36 @@ func (w *WebhookDispatchWorker) Work(ctx context.Context, job *river.Job[service return nil } + tenantID := args.TenantID + if tenantID == nil { + tenantID = service.TenantIDPointerFromEventData(args.Data) + } + + if !service.WebhookMatchesTenant(webhook, tenantID) { + if w.metrics != nil { + w.metrics.RecordDispatchError(ctx, "tenant_mismatch") + } + + var webhookTenantID any + if webhook.TenantID != nil { + webhookTenantID = *webhook.TenantID + } + + var eventTenantID any + if tenantID != nil { + eventTenantID = *tenantID + } + + slog.Error("webhook dispatch: tenant scope mismatch, skipping delivery", + "event_id", args.EventID, + "webhook_id", args.WebhookID, + "webhook_tenant_id", webhookTenantID, + "event_tenant_id", eventTenantID, + ) + + return nil + } + payload := argsToPayload(args) err = w.sender.Send(ctx, webhook, payload) diff --git a/internal/workers/webhook_dispatch_test.go b/internal/workers/webhook_dispatch_test.go index 750a842..91b1eff 100644 --- a/internal/workers/webhook_dispatch_test.go +++ b/internal/workers/webhook_dispatch_test.go @@ -31,10 +31,13 @@ func (m *mockDispatchRepo) Update(_ context.Context, _ uuid.UUID, req *models.Up } type mockSender struct { - err error + err error + calls int } func (m *mockSender) Send(_ context.Context, _ *models.Webhook, _ *service.WebhookPayload) error { + m.calls++ + return m.err } @@ -88,6 +91,97 @@ func TestWebhookDispatchWorker_Work(t *testing.T) { if repo.update != nil { t.Error("Update should not be called on success") } + + if sender.calls != 1 { + t.Errorf("Send called %d times, want 1", sender.calls) + } + }) + + t.Run("returns nil without send when scoped webhook tenant mismatches job tenant", func(t *testing.T) { + webhookTenant := "org-123" + eventTenant := "org-other" + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ + ID: webhookID, + Enabled: true, + URL: "http://x", + SigningKey: "sk", + TenantID: &webhookTenant, + }, + } + sender := &mockSender{} + worker := NewWebhookDispatchWorker(repo, sender, 15*time.Second, nil) + scopedArgs := args + scopedArgs.TenantID = &eventTenant + job := &river.Job[service.WebhookDispatchArgs]{JobRow: &rivertype.JobRow{}, Args: scopedArgs} + + err := worker.Work(ctx, job) + if err != nil { + t.Errorf("Work() error = %v, want nil", err) + } + + if sender.calls != 0 { + t.Errorf("Send called %d times, want 0", sender.calls) + } + + if repo.update != nil { + t.Error("Update should not be called for tenant mismatch") + } + }) + + t.Run("returns nil without send when legacy job data tenant mismatches scoped webhook", func(t *testing.T) { + webhookTenant := "org-123" + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ + ID: webhookID, + Enabled: true, + URL: "http://x", + SigningKey: "sk", + TenantID: &webhookTenant, + }, + } + sender := &mockSender{} + worker := NewWebhookDispatchWorker(repo, sender, 15*time.Second, nil) + scopedArgs := args + scopedArgs.Data = map[string]any{"tenant_id": "org-other"} + scopedArgs.TenantID = nil + job := &river.Job[service.WebhookDispatchArgs]{JobRow: &rivertype.JobRow{}, Args: scopedArgs} + + err := worker.Work(ctx, job) + if err != nil { + t.Errorf("Work() error = %v, want nil", err) + } + + if sender.calls != 0 { + t.Errorf("Send called %d times, want 0", sender.calls) + } + }) + + t.Run("sends when scoped webhook tenant matches job tenant", func(t *testing.T) { + tenantID := "org-123" + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ + ID: webhookID, + Enabled: true, + URL: "http://x", + SigningKey: "sk", + TenantID: &tenantID, + }, + } + sender := &mockSender{} + worker := NewWebhookDispatchWorker(repo, sender, 15*time.Second, nil) + scopedArgs := args + scopedArgs.TenantID = &tenantID + job := &river.Job[service.WebhookDispatchArgs]{JobRow: &rivertype.JobRow{}, Args: scopedArgs} + + err := worker.Work(ctx, job) + if err != nil { + t.Errorf("Work() error = %v, want nil", err) + } + + if sender.calls != 1 { + t.Errorf("Send called %d times, want 1", sender.calls) + } }) t.Run("returns error and does not update when send fails and attempt < max", func(t *testing.T) { From 3ff9589f42447c53519edd6bd46294f4f3908d1e Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Tue, 28 Apr 2026 16:49:18 +0000 Subject: [PATCH 03/12] docs: fix AGENTS markdown spacing --- AGENTS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/AGENTS.md b/AGENTS.md index d8362ee..f75d0fb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -26,6 +26,7 @@ - Keep package names short and domain-focused (e.g., `repository`, `service`). ## Multi-Tenancy & Data Isolation + - Treat `tenant_id` as a security boundary, not a convenience filter. Tenant-owned data must never be read, enqueued, dispatched, cached, searched, embedded, or deleted across tenants. - When making a model, migration, API request, or repository change involving tenant-owned data, audit every downstream path that carries or derives from that data: handlers, services, repositories, message publishers, River job args, workers, webhook payloads, search, embeddings, bulk operations, logs, and metrics. - Prefer tenant-aware repository/service methods for tenant-owned workflows. Avoid adding broad helpers that return all enabled/all matching resources when the caller is dispatching, processing, or exposing tenant data. From f54b8f8831185c0393444f31c06de2733bbcf650 Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Tue, 28 Apr 2026 17:01:49 +0000 Subject: [PATCH 04/12] test(webhooks): keep repository scope test out of unit CI --- .github/workflows/tests.yml | 33 +--- cmd/backfill-embeddings/main.go | 2 +- cmd/worker/main.go | 2 +- internal/config/config.go | 49 ------ internal/config/config_test.go | 66 -------- internal/repository/webhooks_repository.go | 18 ++- .../repository/webhooks_repository_test.go | 145 +++++------------- tests/webhooks_repository_test.go | 130 ++++++++++++++++ 8 files changed, 182 insertions(+), 263 deletions(-) create mode 100644 tests/webhooks_repository_test.go diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 835d04f..e04efba 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -32,16 +32,7 @@ jobs: uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff with: go-version: '1.26.2' - - - name: Cache Go modules - uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 - with: - path: | - ~/.cache/go-build - ~/go/pkg/mod - key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} - restore-keys: | - ${{ runner.os }}-go- + cache-dependency-path: go.sum - name: Install dependencies run: go mod download @@ -86,16 +77,7 @@ jobs: uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff with: go-version: '1.26.2' - - - name: Cache Go modules - uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 - with: - path: | - ~/.cache/go-build - ~/go/pkg/mod - key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} - restore-keys: | - ${{ runner.os }}-go- + cache-dependency-path: go.sum - name: Install dependencies run: go mod download @@ -149,16 +131,7 @@ jobs: uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff with: go-version: '1.26.2' - - - name: Cache Go modules - uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 - with: - path: | - ~/.cache/go-build - ~/go/pkg/mod - key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} - restore-keys: | - ${{ runner.os }}-go- + cache-dependency-path: go.sum - name: Install dependencies run: go mod download diff --git a/cmd/backfill-embeddings/main.go b/cmd/backfill-embeddings/main.go index b522842..8554180 100644 --- a/cmd/backfill-embeddings/main.go +++ b/cmd/backfill-embeddings/main.go @@ -44,7 +44,7 @@ func run() int { return exitFailure } - if cfg.Database.URL == "" || !config.DatabaseURLConfigured() { + if cfg.Database.URL == "" || cfg.Database.URL == config.DefaultDatabaseURL { slog.Error("DATABASE_URL must be set explicitly for this binary (do not use the default test URL)") return exitFailure diff --git a/cmd/worker/main.go b/cmd/worker/main.go index 5660320..88b7eb8 100644 --- a/cmd/worker/main.go +++ b/cmd/worker/main.go @@ -32,7 +32,7 @@ func run() int { return exitFailure } - if cfg.Database.URL == "" || !config.DatabaseURLConfigured() { + if cfg.Database.URL == "" || cfg.Database.URL == config.DefaultDatabaseURL { slog.Error("DATABASE_URL must be set explicitly for hub-worker (do not use the default test URL)") return exitFailure diff --git a/internal/config/config.go b/internal/config/config.go index 8997304..2bd7cd5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -204,55 +204,6 @@ func Load() (*Config, error) { return cfg, nil } -// DatabaseURLConfigured reports whether DATABASE_URL was explicitly set in the -// process environment or local .env file, rather than filled by Config defaults. -func DatabaseURLConfigured() bool { - return databaseURLConfigured(".env") -} - -func databaseURLConfigured(envFile string) bool { - if value, ok := os.LookupEnv("DATABASE_URL"); ok { - return strings.TrimSpace(value) != "" - } - - value, ok := lookupEnvFileValue(envFile, "DATABASE_URL") - if !ok { - return false - } - - return strings.TrimSpace(value) != "" -} - -func lookupEnvFileValue(path, key string) (string, bool) { - data, err := os.ReadFile(path) // #nosec G304 -- runtime reads fixed ".env"; tests pass temp files. - if err != nil { - return "", false - } - - for line := range strings.SplitSeq(string(data), "\n") { - line = strings.TrimSpace(line) - if line == "" || strings.HasPrefix(line, "#") { - continue - } - - line = strings.TrimSpace(strings.TrimPrefix(line, "export ")) - - name, value, ok := strings.Cut(line, "=") - if !ok || strings.TrimSpace(name) != key { - continue - } - - value = strings.TrimSpace(value) - if unquoted, unquoteErr := strconv.Unquote(value); unquoteErr == nil { - value = unquoted - } - - return value, true - } - - return "", false -} - // applyDefaults fills in default values for empty fields (cleanenv may leave nested struct defaults unset). func applyDefaults(cfg *Config) { if cfg.Server.Port == "" { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 00f4449..48b4596 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,8 +1,6 @@ package config import ( - "os" - "path/filepath" "testing" ) @@ -94,70 +92,6 @@ func TestLoadAlwaysReturnsNilError(t *testing.T) { } } -func TestDatabaseURLConfigured(t *testing.T) { - t.Run("true when environment variable is set to default local URL", func(t *testing.T) { - t.Setenv("DATABASE_URL", DefaultDatabaseURL) - - if !databaseURLConfigured(filepath.Join(t.TempDir(), ".env")) { - t.Fatal("databaseURLConfigured() = false, want true") - } - }) - - t.Run("false when environment variable is empty", func(t *testing.T) { - envFile := filepath.Join(t.TempDir(), ".env") - if err := os.WriteFile(envFile, []byte("DATABASE_URL=postgres://from-file\n"), 0o600); err != nil { - t.Fatalf("write env file: %v", err) - } - - t.Setenv("DATABASE_URL", "") - - if databaseURLConfigured(envFile) { - t.Fatal("databaseURLConfigured() = true, want false") - } - }) - - t.Run("true when env file has database URL", func(t *testing.T) { - restoreUnsetEnv(t, "DATABASE_URL") - - envFile := filepath.Join(t.TempDir(), ".env") - if err := os.WriteFile(envFile, []byte("DATABASE_URL=postgres://from-file\n"), 0o600); err != nil { - t.Fatalf("write env file: %v", err) - } - - if !databaseURLConfigured(envFile) { - t.Fatal("databaseURLConfigured() = false, want true") - } - }) - - t.Run("false when neither env nor env file has database URL", func(t *testing.T) { - restoreUnsetEnv(t, "DATABASE_URL") - - if databaseURLConfigured(filepath.Join(t.TempDir(), ".env")) { - t.Fatal("databaseURLConfigured() = true, want false") - } - }) -} - -//nolint:usetesting // This helper deliberately unsets env vars; t.Setenv cannot express an unset value. -func restoreUnsetEnv(t *testing.T, key string) { - t.Helper() - - oldValue, hadValue := os.LookupEnv(key) - if err := os.Unsetenv(key); err != nil { - t.Fatalf("unset %s: %v", key, err) - } - - t.Cleanup(func() { - if hadValue { - _ = os.Setenv(key, oldValue) - - return - } - - _ = os.Unsetenv(key) - }) -} - func TestLoad_WebhookDeliveryMaxAttempts(t *testing.T) { t.Run("default is 3 when unset", func(t *testing.T) { cfg, err := Load() diff --git a/internal/repository/webhooks_repository.go b/internal/repository/webhooks_repository.go index 7e5f627..7c6f367 100644 --- a/internal/repository/webhooks_repository.go +++ b/internal/repository/webhooks_repository.go @@ -401,6 +401,17 @@ const listEnabledForEventTypeSelect = ` func (r *WebhooksRepository) ListEnabledForEventTypeAndTenant( ctx context.Context, eventType string, tenantID *string, ) ([]models.Webhook, error) { + query, args := listEnabledForEventTypeAndTenantQuery(eventType, tenantID) + + webhooks, err := r.fetchWebhooks(ctx, query, args...) + if err != nil { + return nil, fmt.Errorf("list enabled webhooks for event type and tenant: %w", err) + } + + return webhooks, nil +} + +func listEnabledForEventTypeAndTenantQuery(eventType string, tenantID *string) (string, []any) { query := listEnabledForEventTypeSelect args := []any{eventType} @@ -414,12 +425,7 @@ func (r *WebhooksRepository) ListEnabledForEventTypeAndTenant( query += ` ORDER BY id` - webhooks, err := r.fetchWebhooks(ctx, query, args...) - if err != nil { - return nil, fmt.Errorf("list enabled webhooks for event type and tenant: %w", err) - } - - return webhooks, nil + return query, args } // fetchWebhooks executes the given query and scans rows into Webhook slices. diff --git a/internal/repository/webhooks_repository_test.go b/internal/repository/webhooks_repository_test.go index b99d5cf..29214aa 100644 --- a/internal/repository/webhooks_repository_test.go +++ b/internal/repository/webhooks_repository_test.go @@ -1,126 +1,51 @@ package repository import ( - "context" - "os" "strings" "testing" - "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/formbricks/hub/internal/config" "github.com/formbricks/hub/internal/datatypes" - "github.com/formbricks/hub/internal/models" - "github.com/formbricks/hub/pkg/database" ) -const repositoryTestDatabaseURL = "postgres://postgres:postgres@localhost:5432/test_db?sslmode=disable" - -func TestWebhooksRepository_ListEnabledForEventTypeAndTenant(t *testing.T) { - ctx := context.Background() - - databaseURL := os.Getenv("DATABASE_URL") - if databaseURL == "" { - databaseURL = repositoryTestDatabaseURL +func TestListEnabledForEventTypeAndTenantQuery(t *testing.T) { + tenantID := "tenant-a" + + tests := []struct { + name string + tenantID *string + wantArgs []any + wantTenantClause string + rejectTenantClause string + }{ + { + name: "scoped event matches global or same tenant webhooks", + tenantID: &tenantID, + wantArgs: []any{datatypes.FeedbackRecordCreated.String(), tenantID}, + wantTenantClause: "AND (tenant_id IS NULL OR tenant_id = $2)", + rejectTenantClause: "AND tenant_id IS NULL ORDER BY id", + }, + { + name: "tenant-less event matches global webhooks only", + tenantID: nil, + wantArgs: []any{datatypes.FeedbackRecordCreated.String()}, + wantTenantClause: "AND tenant_id IS NULL", + rejectTenantClause: "tenant_id = $2", + }, } - t.Setenv("DATABASE_URL", databaseURL) - - cfg, err := config.Load() - require.NoError(t, err) - db, err := database.NewPostgresPool(ctx, cfg.Database.URL, - database.WithPoolConfig(cfg.Database.PoolConfig()), - ) - require.NoError(t, err) - - defer db.Close() - - _, err = db.Exec(ctx, "DELETE FROM webhooks WHERE url LIKE 'https://tenant-scope.test/%'") - require.NoError(t, err) - - defer func() { - _, cleanupErr := db.Exec(ctx, "DELETE FROM webhooks WHERE url LIKE 'https://tenant-scope.test/%'") - require.NoError(t, cleanupErr) - }() - - repo := NewWebhooksRepository(db) - tenantA := "repo-scope-tenant-a" - tenantB := "repo-scope-tenant-b" - disabled := false - feedbackCreated := []datatypes.EventType{datatypes.FeedbackRecordCreated} - feedbackUpdated := []datatypes.EventType{datatypes.FeedbackRecordUpdated} - - globalWebhook := createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/global", nil, nil) - tenantAWebhook := createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/tenant-a", &tenantA, feedbackCreated) - tenantBWebhook := createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/tenant-b", &tenantB, feedbackCreated) - disabledTenantAWebhook := createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/disabled-a", &tenantA, feedbackCreated) - _, err = repo.Update(ctx, disabledTenantAWebhook.ID, &models.UpdateWebhookRequest{Enabled: &disabled}) - require.NoError(t, err) - createWebhookForRepositoryTest(ctx, t, repo, "https://tenant-scope.test/updated-only-a", &tenantA, feedbackUpdated) - - tenantAWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), &tenantA) - require.NoError(t, err) - assertRepositoryWebhookIDs(t, tenantAWebhooks, map[uuid.UUID]bool{ - globalWebhook.ID: true, - tenantAWebhook.ID: true, - }) - - tenantBWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), &tenantB) - require.NoError(t, err) - assertRepositoryWebhookIDs(t, tenantBWebhooks, map[uuid.UUID]bool{ - globalWebhook.ID: true, - tenantBWebhook.ID: true, - }) - - globalOnlyWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), nil) - require.NoError(t, err) - assertRepositoryWebhookIDs(t, globalOnlyWebhooks, map[uuid.UUID]bool{ - globalWebhook.ID: true, - }) -} - -func createWebhookForRepositoryTest( - ctx context.Context, - t *testing.T, - repo *WebhooksRepository, - url string, - tenantID *string, - eventTypes []datatypes.EventType, -) *models.Webhook { - t.Helper() - - webhook, err := repo.Create(ctx, &models.CreateWebhookRequest{ - URL: url, - SigningKey: "whsec_abcdefghijklmnopqrstuvwxyz123456", - TenantID: tenantID, - EventTypes: eventTypes, - }) - require.NoError(t, err) - - return webhook -} - -func assertRepositoryWebhookIDs(t *testing.T, webhooks []models.Webhook, wantIDs map[uuid.UUID]bool) { - t.Helper() - - gotIDs := make(map[uuid.UUID]bool, len(webhooks)) - for _, webhook := range webhooks { - if !strings.HasPrefix(webhook.URL, "https://tenant-scope.test/") { - continue - } - - if !wantIDs[webhook.ID] { - t.Fatalf("unexpected scoped test webhook returned: %+v", webhook) - } - - gotIDs[webhook.ID] = true - } - - assert.Len(t, gotIDs, len(wantIDs), "webhooks = %+v", webhooks) - - for id := range wantIDs { - assert.True(t, gotIDs[id], "missing webhook %s in %+v", id, webhooks) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + query, args := listEnabledForEventTypeAndTenantQuery(datatypes.FeedbackRecordCreated.String(), tt.tenantID) + + require.Equal(t, tt.wantArgs, args) + assert.Contains(t, query, "WHERE enabled = true") + assert.Contains(t, query, "event_types IS NULL OR event_types = '{}' OR event_types @> ARRAY[$1]::VARCHAR(64)[]") + assert.Contains(t, query, tt.wantTenantClause) + assert.NotContains(t, query, tt.rejectTenantClause) + assert.True(t, strings.HasSuffix(strings.TrimSpace(query), "ORDER BY id")) + }) } } diff --git a/tests/webhooks_repository_test.go b/tests/webhooks_repository_test.go new file mode 100644 index 0000000..8b3b4a1 --- /dev/null +++ b/tests/webhooks_repository_test.go @@ -0,0 +1,130 @@ +package tests + +import ( + "context" + "os" + "strings" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/formbricks/hub/internal/config" + "github.com/formbricks/hub/internal/datatypes" + "github.com/formbricks/hub/internal/models" + "github.com/formbricks/hub/internal/repository" + "github.com/formbricks/hub/pkg/database" +) + +const repositoryWebhookScopeURLPrefix = "https://tenant-scope.test/" + +func TestWebhooksRepository_ListEnabledForEventTypeAndTenant(t *testing.T) { + ctx := context.Background() + + databaseURL := os.Getenv("DATABASE_URL") + if databaseURL == "" { + databaseURL = defaultTestDatabaseURL + } + + t.Setenv("API_KEY", testAPIKey) + t.Setenv("DATABASE_URL", databaseURL) + + cfg, err := config.Load() + require.NoError(t, err) + + db, err := database.NewPostgresPool(ctx, cfg.Database.URL, + database.WithPoolConfig(cfg.Database.PoolConfig()), + ) + require.NoError(t, err) + + defer db.Close() + + cleanupRepositoryWebhookScopeTestRows := func() { + _, cleanupErr := db.Exec(ctx, "DELETE FROM webhooks WHERE url LIKE $1", repositoryWebhookScopeURLPrefix+"%") + require.NoError(t, cleanupErr) + } + + cleanupRepositoryWebhookScopeTestRows() + defer cleanupRepositoryWebhookScopeTestRows() + + repo := repository.NewWebhooksRepository(db) + tenantA := "repo-scope-tenant-a" + tenantB := "repo-scope-tenant-b" + disabled := false + feedbackCreated := []datatypes.EventType{datatypes.FeedbackRecordCreated} + feedbackUpdated := []datatypes.EventType{datatypes.FeedbackRecordUpdated} + + globalWebhook := createWebhookForRepositoryScopeTest(ctx, t, repo, "global", nil, nil) + tenantAWebhook := createWebhookForRepositoryScopeTest(ctx, t, repo, "tenant-a", &tenantA, feedbackCreated) + tenantBWebhook := createWebhookForRepositoryScopeTest(ctx, t, repo, "tenant-b", &tenantB, feedbackCreated) + disabledTenantAWebhook := createWebhookForRepositoryScopeTest(ctx, t, repo, "disabled-a", &tenantA, feedbackCreated) + _, err = repo.Update(ctx, disabledTenantAWebhook.ID, &models.UpdateWebhookRequest{Enabled: &disabled}) + require.NoError(t, err) + + createWebhookForRepositoryScopeTest(ctx, t, repo, "updated-only-a", &tenantA, feedbackUpdated) + + tenantAWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), &tenantA) + require.NoError(t, err) + assertRepositoryScopeWebhookIDs(t, tenantAWebhooks, map[uuid.UUID]bool{ + globalWebhook.ID: true, + tenantAWebhook.ID: true, + }) + + tenantBWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), &tenantB) + require.NoError(t, err) + assertRepositoryScopeWebhookIDs(t, tenantBWebhooks, map[uuid.UUID]bool{ + globalWebhook.ID: true, + tenantBWebhook.ID: true, + }) + + globalOnlyWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), nil) + require.NoError(t, err) + assertRepositoryScopeWebhookIDs(t, globalOnlyWebhooks, map[uuid.UUID]bool{ + globalWebhook.ID: true, + }) +} + +func createWebhookForRepositoryScopeTest( + ctx context.Context, + t *testing.T, + repo *repository.WebhooksRepository, + path string, + tenantID *string, + eventTypes []datatypes.EventType, +) *models.Webhook { + t.Helper() + + webhook, err := repo.Create(ctx, &models.CreateWebhookRequest{ + URL: repositoryWebhookScopeURLPrefix + path, + SigningKey: "whsec_abcdefghijklmnopqrstuvwxyz123456", + TenantID: tenantID, + EventTypes: eventTypes, + }) + require.NoError(t, err) + + return webhook +} + +func assertRepositoryScopeWebhookIDs(t *testing.T, webhooks []models.Webhook, wantIDs map[uuid.UUID]bool) { + t.Helper() + + gotIDs := make(map[uuid.UUID]bool, len(webhooks)) + for _, webhook := range webhooks { + if !strings.HasPrefix(webhook.URL, repositoryWebhookScopeURLPrefix) { + continue + } + + if !wantIDs[webhook.ID] { + t.Fatalf("unexpected scoped test webhook returned: %+v", webhook) + } + + gotIDs[webhook.ID] = true + } + + assert.Len(t, gotIDs, len(wantIDs), "webhooks = %+v", webhooks) + + for id := range wantIDs { + assert.True(t, gotIDs[id], "missing webhook %s in %+v", id, webhooks) + } +} From 5002fa1b7cbdc68e81ce1018db745a1c04cadcf3 Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Tue, 28 Apr 2026 17:10:44 +0000 Subject: [PATCH 05/12] chore: restore tests workflow --- .github/workflows/tests.yml | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index e04efba..835d04f 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -32,7 +32,16 @@ jobs: uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff with: go-version: '1.26.2' - cache-dependency-path: go.sum + + - name: Cache Go modules + uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 + with: + path: | + ~/.cache/go-build + ~/go/pkg/mod + key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} + restore-keys: | + ${{ runner.os }}-go- - name: Install dependencies run: go mod download @@ -77,7 +86,16 @@ jobs: uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff with: go-version: '1.26.2' - cache-dependency-path: go.sum + + - name: Cache Go modules + uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 + with: + path: | + ~/.cache/go-build + ~/go/pkg/mod + key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} + restore-keys: | + ${{ runner.os }}-go- - name: Install dependencies run: go mod download @@ -131,7 +149,16 @@ jobs: uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff with: go-version: '1.26.2' - cache-dependency-path: go.sum + + - name: Cache Go modules + uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 + with: + path: | + ~/.cache/go-build + ~/go/pkg/mod + key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} + restore-keys: | + ${{ runner.os }}-go- - name: Install dependencies run: go mod download From b2028c3703cf2bfbeac281fb4e41cafcc299664d Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Tue, 28 Apr 2026 21:22:28 +0000 Subject: [PATCH 06/12] fix: tighten webhook tenant isolation --- AGENTS.md | 9 +- .../api/handlers/feedback_records_handler.go | 22 ++- .../handlers/feedback_records_handler_test.go | 28 +++- internal/models/events.go | 9 ++ internal/models/feedback_records.go | 2 +- internal/models/webhooks.go | 4 +- internal/observability/names.go | 5 +- .../repository/feedback_records_repository.go | 18 +-- internal/repository/webhooks_repository.go | 33 +--- .../repository/webhooks_repository_test.go | 10 +- internal/service/feedback_records_service.go | 30 +++- .../service/feedback_records_service_test.go | 147 ++++++++++++++++++ internal/service/tenant_validation.go | 20 +++ internal/service/webhook_payload.go | 51 ++++++ internal/service/webhook_payload_test.go | 66 ++++++++ internal/service/webhook_provider.go | 24 ++- internal/service/webhook_provider_test.go | 101 +++--------- internal/service/webhook_sender.go | 10 +- internal/service/webhook_sender_test.go | 36 ----- internal/service/webhook_tenant.go | 14 +- internal/service/webhook_tenant_test.go | 10 +- internal/service/webhooks_service.go | 59 ++++++- internal/service/webhooks_service_test.go | 113 ++++++++++++-- internal/workers/webhook_dispatch.go | 35 +++-- internal/workers/webhook_dispatch_test.go | 86 ++++++++-- migrations/008_require_webhook_tenant_id.sql | 18 +++ openapi.yaml | 32 +++- tests/integration_test.go | 65 ++++++-- tests/webhooks_repository_test.go | 33 ++-- 29 files changed, 820 insertions(+), 270 deletions(-) create mode 100644 internal/models/events.go create mode 100644 internal/service/feedback_records_service_test.go create mode 100644 internal/service/tenant_validation.go create mode 100644 internal/service/webhook_payload_test.go create mode 100644 migrations/008_require_webhook_tenant_id.sql diff --git a/AGENTS.md b/AGENTS.md index f75d0fb..c05ec8e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -29,10 +29,13 @@ - Treat `tenant_id` as a security boundary, not a convenience filter. Tenant-owned data must never be read, enqueued, dispatched, cached, searched, embedded, or deleted across tenants. - When making a model, migration, API request, or repository change involving tenant-owned data, audit every downstream path that carries or derives from that data: handlers, services, repositories, message publishers, River job args, workers, webhook payloads, search, embeddings, bulk operations, logs, and metrics. -- Prefer tenant-aware repository/service methods for tenant-owned workflows. Avoid adding broad helpers that return all enabled/all matching resources when the caller is dispatching, processing, or exposing tenant data. +- Tenant access rules must be consistent across every path that can observe, mutate, derive from, or act on the same resource. If one API endpoint, repository method, search path, webhook dispatch path, worker, backfill, bulk operation, or export path requires tenant scope, every alternate path for that resource must enforce the same tenant boundary. +- Do not model `tenant_id` as an optional filter for tenant-owned resources. Prefer required tenant parameters in service/repository method signatures (`tenantID string`, not `*string`) unless the domain explicitly supports global resources and documents that behavior. +- Prefer tenant-aware repository/service methods for tenant-owned workflows. Avoid adding broad helpers that return all enabled/all matching resources when the caller is dispatching, processing, deriving, exporting, or exposing tenant data. - Async jobs must carry the tenant boundary when the source data has one, and workers must re-check tenant scope before doing side effects. Do not rely only on enqueue-time filtering. -- Global resources may intentionally have `tenant_id = NULL`, but code must make that behavior explicit. A missing tenant on event data should not match tenant-scoped resources. -- For any tenant-scoping change, include verification at the boundary where the leak could happen: database query behavior, service fan-out, worker execution, and API behavior when relevant. Tests are the evidence; the invariant belongs in the architecture. +- Global resources may intentionally have `tenant_id = NULL` only when the domain explicitly documents them as non-tenant-owned. Webhooks are tenant-owned and must require a non-empty `tenant_id`; a missing tenant on event data must not match any webhook. +- When changing access rules for a tenant-owned model, search for all alternate access paths by resource name and by derived side effects: list, get, update, delete, bulk delete, webhook fan-out, River jobs, workers, embeddings, search, exports, cache invalidation, logs, and metrics. +- For any tenant-scoping change, include verification at the boundary where the leak could happen: database query behavior, service fan-out, worker execution, and API behavior when relevant. Include at least one alternate-path regression test proving that data allowed through the primary path cannot leak through async dispatch, bulk operations, derived indexes, exports, or background workers. Tests are the evidence; the invariant belongs in the architecture. ## Testing Guidelines - Tests live under `tests/` and are run with `go test ./tests/...`. diff --git a/internal/api/handlers/feedback_records_handler.go b/internal/api/handlers/feedback_records_handler.go index 0524ec7..c085157 100644 --- a/internal/api/handlers/feedback_records_handler.go +++ b/internal/api/handlers/feedback_records_handler.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "net/http" "github.com/google/uuid" @@ -219,7 +220,7 @@ func (h *FeedbackRecordsHandler) Delete(w http.ResponseWriter, r *http.Request) w.WriteHeader(http.StatusNoContent) } -// BulkDelete handles DELETE /v1/feedback-records?user_id=. +// BulkDelete handles DELETE /v1/feedback-records?user_id=&tenant_id=. func (h *FeedbackRecordsHandler) BulkDelete(w http.ResponseWriter, r *http.Request) { filters := &models.BulkDeleteFilters{} @@ -232,6 +233,25 @@ func (h *FeedbackRecordsHandler) BulkDelete(w http.ResponseWriter, r *http.Reque deletedCount, err := h.service.BulkDeleteFeedbackRecords(r.Context(), filters.UserID, filters.TenantID) if err != nil { + if errors.Is(err, huberrors.ErrValidation) { + validation.RespondValidationError(w, err) + + return + } + + var tenantID string + if filters.TenantID != nil { + tenantID = *filters.TenantID + } + + slog.Error("Failed to bulk delete feedback records", // #nosec G706 -- slog key-values + "method", r.Method, + "path", r.URL.Path, + "user_id", filters.UserID, + "tenant_id", tenantID, + "error", err, + ) + response.RespondInternalServerError(w, "An unexpected error occurred") return diff --git a/internal/api/handlers/feedback_records_handler_test.go b/internal/api/handlers/feedback_records_handler_test.go index 44aa1a7..24632f7 100644 --- a/internal/api/handlers/feedback_records_handler_test.go +++ b/internal/api/handlers/feedback_records_handler_test.go @@ -70,8 +70,10 @@ func TestFeedbackRecordsHandler_List(t *testing.T) { func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { t.Run("success returns 200 with deleted_count and message", func(t *testing.T) { mock := &mockFeedbackRecordsService{ - bulkDeleteFunc: func(_ context.Context, userID string, _ *string) (int, error) { + bulkDeleteFunc: func(_ context.Context, userID string, tenantID *string) (int, error) { assert.Equal(t, "user-123", userID) + require.NotNil(t, tenantID) + assert.Equal(t, "tenant-a", *tenantID) return 3, nil }, @@ -79,7 +81,7 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { handler := NewFeedbackRecordsHandler(mock) req := httptest.NewRequestWithContext(context.Background(), - http.MethodDelete, "http://test/v1/feedback-records?user_id=user-123", http.NoBody) + http.MethodDelete, "http://test/v1/feedback-records?user_id=user-123&tenant_id=tenant-a", http.NoBody) rec := httptest.NewRecorder() handler.BulkDelete(rec, req) @@ -119,11 +121,25 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { assert.Equal(t, "tenant-a", *capturedTenantID) }) + t.Run("missing tenant_id returns bad request", func(t *testing.T) { + mock := &mockFeedbackRecordsService{} + handler := NewFeedbackRecordsHandler(mock) + + req := httptest.NewRequestWithContext(context.Background(), + http.MethodDelete, "http://test/v1/feedback-records?user_id=user-123", http.NoBody) + rec := httptest.NewRecorder() + + handler.BulkDelete(rec, req) + + assert.Equal(t, http.StatusBadRequest, rec.Code) + }) + t.Run("missing user_id returns bad request", func(t *testing.T) { mock := &mockFeedbackRecordsService{} handler := NewFeedbackRecordsHandler(mock) - req := httptest.NewRequestWithContext(context.Background(), http.MethodDelete, "http://test/v1/feedback-records", http.NoBody) + req := httptest.NewRequestWithContext(context.Background(), + http.MethodDelete, "http://test/v1/feedback-records?tenant_id=tenant-a", http.NoBody) rec := httptest.NewRecorder() handler.BulkDelete(rec, req) @@ -136,7 +152,7 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { handler := NewFeedbackRecordsHandler(mock) req := httptest.NewRequestWithContext(context.Background(), - http.MethodDelete, "http://test/v1/feedback-records?user_id=", http.NoBody) + http.MethodDelete, "http://test/v1/feedback-records?user_id=&tenant_id=tenant-a", http.NoBody) rec := httptest.NewRecorder() handler.BulkDelete(rec, req) @@ -153,7 +169,7 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { handler := NewFeedbackRecordsHandler(mock) req := httptest.NewRequestWithContext(context.Background(), - http.MethodDelete, "http://test/v1/feedback-records?user_id=user-789", http.NoBody) + http.MethodDelete, "http://test/v1/feedback-records?user_id=user-789&tenant_id=tenant-a", http.NoBody) rec := httptest.NewRecorder() handler.BulkDelete(rec, req) @@ -170,7 +186,7 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { handler := NewFeedbackRecordsHandler(mock) req := httptest.NewRequestWithContext(context.Background(), - http.MethodDelete, "http://test/v1/feedback-records?user_id=nonexistent", http.NoBody) + http.MethodDelete, "http://test/v1/feedback-records?user_id=nonexistent&tenant_id=tenant-a", http.NoBody) rec := httptest.NewRecorder() handler.BulkDelete(rec, req) diff --git a/internal/models/events.go b/internal/models/events.go new file mode 100644 index 0000000..4a9b8f2 --- /dev/null +++ b/internal/models/events.go @@ -0,0 +1,9 @@ +package models + +import "github.com/google/uuid" + +// DeletedIDsEventData is the tenant-aware payload for resource deletion events. +type DeletedIDsEventData struct { + TenantID string `json:"tenant_id"` + IDs []uuid.UUID `json:"ids"` +} diff --git a/internal/models/feedback_records.go b/internal/models/feedback_records.go index 39e601e..c2805b6 100644 --- a/internal/models/feedback_records.go +++ b/internal/models/feedback_records.go @@ -199,7 +199,7 @@ type ListFeedbackRecordsResponse struct { // BulkDeleteFilters represents query parameters for bulk delete operation. type BulkDeleteFilters struct { UserID string `form:"user_id" validate:"required,no_null_bytes,min=1"` - TenantID *string `form:"tenant_id" validate:"omitempty,no_null_bytes"` + TenantID *string `form:"tenant_id" validate:"required,no_null_bytes,min=1"` } // BulkDeleteResponse represents the response for bulk delete operation. diff --git a/internal/models/webhooks.go b/internal/models/webhooks.go index d5e84bd..5f2931d 100644 --- a/internal/models/webhooks.go +++ b/internal/models/webhooks.go @@ -183,7 +183,7 @@ type CreateWebhookRequest struct { URL string `json:"url" validate:"required,no_null_bytes,http_url,min=1,max=2048"` SigningKey string `json:"signing_key,omitempty" validate:"omitempty,max=255"` Enabled *bool `json:"enabled,omitempty"` - TenantID *string `json:"tenant_id,omitempty" validate:"omitempty,no_null_bytes,max=255"` + TenantID *string `json:"tenant_id" validate:"required,no_null_bytes,min=1,max=255"` EventTypes []datatypes.EventType `json:"event_types,omitempty"` } @@ -219,7 +219,7 @@ type UpdateWebhookRequest struct { URL *string `json:"url,omitempty" validate:"omitempty,no_null_bytes,http_url,min=1,max=2048"` SigningKey *string `json:"signing_key,omitempty" validate:"omitempty,no_null_bytes,min=1,max=255"` Enabled *bool `json:"enabled,omitempty"` - TenantID *string `json:"tenant_id,omitempty" validate:"omitempty,no_null_bytes,max=255"` + TenantID *string `json:"tenant_id,omitempty" validate:"omitempty,no_null_bytes,min=1,max=255"` EventTypes *[]datatypes.EventType `json:"event_types,omitempty"` DisabledReason *string `json:"-"` // read-only; set by system when disabling DisabledAt *time.Time `json:"-"` // read-only; set by system when disabling diff --git a/internal/observability/names.go b/internal/observability/names.go index feecf27..6e0e750 100644 --- a/internal/observability/names.go +++ b/internal/observability/names.go @@ -43,8 +43,9 @@ func AllowedEventTypes() []string { // allowedProviderReasons for hub_webhook_provider_errors_total (bounded cardinality). var allowedProviderReasons = map[string]bool{ - "list_failed": true, - "enqueue_failed": true, + "list_failed": true, + "enqueue_failed": true, + "missing_tenant_id": true, } // allowedDeliveryStatuses for hub_webhook_deliveries_total and hub_webhook_delivery_duration_seconds. diff --git a/internal/repository/feedback_records_repository.go b/internal/repository/feedback_records_repository.go index 678c122..247abb4 100644 --- a/internal/repository/feedback_records_repository.go +++ b/internal/repository/feedback_records_repository.go @@ -394,22 +394,14 @@ func (r *FeedbackRecordsRepository) Delete(ctx context.Context, id uuid.UUID) er return nil } -// BulkDelete deletes all feedback records matching user_id and optional tenant_id. +// BulkDelete deletes all feedback records matching user_id and tenant_id. // It returns the deleted IDs (via RETURNING id) so callers can e.g. publish events. -func (r *FeedbackRecordsRepository) BulkDelete(ctx context.Context, userID string, tenantID *string) ([]uuid.UUID, error) { +func (r *FeedbackRecordsRepository) BulkDelete(ctx context.Context, userID, tenantID string) ([]uuid.UUID, error) { query := ` DELETE FROM feedback_records - WHERE user_id = $1` - args := []any{userID} - argCount := 2 - - if tenantID != nil { - query += fmt.Sprintf(" AND tenant_id = $%d", argCount) - - args = append(args, *tenantID) - } - - query += ` RETURNING id` + WHERE user_id = $1 AND tenant_id = $2 + RETURNING id` + args := []any{userID, tenantID} rows, err := r.db.Query(ctx, query, args...) if err != nil { diff --git a/internal/repository/webhooks_repository.go b/internal/repository/webhooks_repository.go index 7c6f367..67d3793 100644 --- a/internal/repository/webhooks_repository.go +++ b/internal/repository/webhooks_repository.go @@ -262,16 +262,8 @@ func (r *WebhooksRepository) Update(ctx context.Context, id uuid.UUID, req *mode } if req.TenantID != nil { - // Empty string clears tenant_id (store as NULL) - var val any - if *req.TenantID == "" { - val = nil - } else { - val = *req.TenantID - } - updates = append(updates, fmt.Sprintf("tenant_id = $%d", argCount)) - args = append(args, val) + args = append(args, *req.TenantID) argCount++ } @@ -376,28 +368,15 @@ func parseDBEventTypes(ss []string) ([]datatypes.EventType, error) { return out, nil } -// ListEnabled retrieves all enabled webhooks (unbounded; used for delivery fan-out). -func (r *WebhooksRepository) ListEnabled(ctx context.Context) ([]models.Webhook, error) { - query := webhooksListSelect + ` WHERE enabled = true ORDER BY created_at DESC, id ASC` - - webhooks, err := r.fetchWebhooks(ctx, query) - if err != nil { - return nil, fmt.Errorf("list enabled webhooks: %w", err) - } - - return webhooks, nil -} - const listEnabledForEventTypeSelect = ` - SELECT id, url, signing_key, enabled, tenant_id, created_at, updated_at, event_types, disabled_reason, disabled_at - FROM webhooks + SELECT id, url, signing_key, enabled, tenant_id, created_at, updated_at, event_types, disabled_reason, disabled_at + FROM webhooks WHERE enabled = true AND (event_types IS NULL OR event_types = '{}' OR event_types @> ARRAY[$1]::VARCHAR(64)[]) ` // ListEnabledForEventTypeAndTenant retrieves enabled webhooks for an event type and tenant boundary. -// Global webhooks (tenant_id NULL) match every event. Tenant-scoped webhooks match only the same tenant. -// When tenantID is nil, only global webhooks are returned because scoped webhooks cannot be proven safe. +// Webhooks match only the same tenant. A missing tenantID matches nothing. func (r *WebhooksRepository) ListEnabledForEventTypeAndTenant( ctx context.Context, eventType string, tenantID *string, ) ([]models.Webhook, error) { @@ -416,9 +395,9 @@ func listEnabledForEventTypeAndTenantQuery(eventType string, tenantID *string) ( args := []any{eventType} if tenantID == nil { - query += ` AND tenant_id IS NULL` + query += ` AND FALSE` } else { - query += ` AND (tenant_id IS NULL OR tenant_id = $2)` + query += ` AND tenant_id = $2` args = append(args, *tenantID) } diff --git a/internal/repository/webhooks_repository_test.go b/internal/repository/webhooks_repository_test.go index 29214aa..5ea7704 100644 --- a/internal/repository/webhooks_repository_test.go +++ b/internal/repository/webhooks_repository_test.go @@ -21,17 +21,17 @@ func TestListEnabledForEventTypeAndTenantQuery(t *testing.T) { rejectTenantClause string }{ { - name: "scoped event matches global or same tenant webhooks", + name: "scoped event matches same tenant webhooks only", tenantID: &tenantID, wantArgs: []any{datatypes.FeedbackRecordCreated.String(), tenantID}, - wantTenantClause: "AND (tenant_id IS NULL OR tenant_id = $2)", - rejectTenantClause: "AND tenant_id IS NULL ORDER BY id", + wantTenantClause: "AND tenant_id = $2", + rejectTenantClause: "tenant_id IS NULL", }, { - name: "tenant-less event matches global webhooks only", + name: "tenant-less event matches no webhooks", tenantID: nil, wantArgs: []any{datatypes.FeedbackRecordCreated.String()}, - wantTenantClause: "AND tenant_id IS NULL", + wantTenantClause: "AND FALSE", rejectTenantClause: "tenant_id = $2", }, } diff --git a/internal/service/feedback_records_service.go b/internal/service/feedback_records_service.go index 8b279e9..2fd3ffa 100644 --- a/internal/service/feedback_records_service.go +++ b/internal/service/feedback_records_service.go @@ -35,7 +35,7 @@ type FeedbackRecordsRepository interface { ) ([]models.FeedbackRecord, bool, error) Update(ctx context.Context, id uuid.UUID, req *models.UpdateFeedbackRecordRequest) (*models.FeedbackRecord, error) Delete(ctx context.Context, id uuid.UUID) error - BulkDelete(ctx context.Context, userID string, tenantID *string) ([]uuid.UUID, error) + BulkDelete(ctx context.Context, userID, tenantID string) ([]uuid.UUID, error) } // EmbeddingsRepository defines the interface for embeddings table access. @@ -182,33 +182,49 @@ func (s *FeedbackRecordsService) UpdateFeedbackRecord( } // DeleteFeedbackRecord deletes a feedback record by ID. -// Publishes FeedbackRecordDeleted with data = [id] (array of deleted IDs) for consistency with bulk delete. +// Publishes FeedbackRecordDeleted with tenant-aware deleted IDs for webhook isolation. func (s *FeedbackRecordsService) DeleteFeedbackRecord(ctx context.Context, id uuid.UUID) error { + record, err := s.repo.GetByID(ctx, id) + if err != nil { + return fmt.Errorf("get feedback record before delete: %w", err) + } + if err := s.repo.Delete(ctx, id); err != nil { return fmt.Errorf("delete feedback record: %w", err) } if s.publisher != nil { - s.publisher.PublishEvent(ctx, datatypes.FeedbackRecordDeleted, []uuid.UUID{id}) + s.publisher.PublishEvent(ctx, datatypes.FeedbackRecordDeleted, models.DeletedIDsEventData{ + TenantID: record.TenantID, + IDs: []uuid.UUID{id}, + }) } return nil } -// BulkDeleteFeedbackRecords deletes all feedback records matching user_id and optional tenant_id. -// Publishes a single FeedbackRecordDeleted event with data = [id1, id2, ...] (array of deleted IDs). +// BulkDeleteFeedbackRecords deletes all feedback records matching user_id and tenant_id. +// Publishes a single tenant-aware FeedbackRecordDeleted event. func (s *FeedbackRecordsService) BulkDeleteFeedbackRecords(ctx context.Context, userID string, tenantID *string) (int, error) { if userID == "" { return 0, ErrUserIDRequired } - ids, err := s.repo.BulkDelete(ctx, userID, tenantID) + normalizedTenantID, err := normalizeRequiredTenantID(tenantID) + if err != nil { + return 0, err + } + + ids, err := s.repo.BulkDelete(ctx, userID, normalizedTenantID) if err != nil { return 0, fmt.Errorf("bulk delete feedback records: %w", err) } if len(ids) > 0 && s.publisher != nil { - s.publisher.PublishEvent(ctx, datatypes.FeedbackRecordDeleted, ids) + s.publisher.PublishEvent(ctx, datatypes.FeedbackRecordDeleted, models.DeletedIDsEventData{ + TenantID: normalizedTenantID, + IDs: ids, + }) } return len(ids), nil diff --git a/internal/service/feedback_records_service_test.go b/internal/service/feedback_records_service_test.go new file mode 100644 index 0000000..8f8b2e9 --- /dev/null +++ b/internal/service/feedback_records_service_test.go @@ -0,0 +1,147 @@ +package service + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/google/uuid" + + "github.com/formbricks/hub/internal/datatypes" + "github.com/formbricks/hub/internal/huberrors" + "github.com/formbricks/hub/internal/models" +) + +type mockFeedbackRecordsRepo struct { + record *models.FeedbackRecord + bulkIDs []uuid.UUID + deletedID uuid.UUID +} + +func (m *mockFeedbackRecordsRepo) Create( + _ context.Context, _ *models.CreateFeedbackRecordRequest, +) (*models.FeedbackRecord, error) { + return nil, errors.New("not implemented") +} + +func (m *mockFeedbackRecordsRepo) GetByID(_ context.Context, _ uuid.UUID) (*models.FeedbackRecord, error) { + return m.record, nil +} + +func (m *mockFeedbackRecordsRepo) List( + _ context.Context, _ *models.ListFeedbackRecordsFilters, +) ([]models.FeedbackRecord, bool, error) { + return nil, false, errors.New("not implemented") +} + +func (m *mockFeedbackRecordsRepo) ListAfterCursor( + _ context.Context, _ *models.ListFeedbackRecordsFilters, _ time.Time, _ uuid.UUID, +) ([]models.FeedbackRecord, bool, error) { + return nil, false, errors.New("not implemented") +} + +func (m *mockFeedbackRecordsRepo) Update( + _ context.Context, _ uuid.UUID, _ *models.UpdateFeedbackRecordRequest, +) (*models.FeedbackRecord, error) { + return nil, errors.New("not implemented") +} + +func (m *mockFeedbackRecordsRepo) Delete(_ context.Context, id uuid.UUID) error { + m.deletedID = id + + return nil +} + +func (m *mockFeedbackRecordsRepo) BulkDelete(_ context.Context, _, _ string) ([]uuid.UUID, error) { + return m.bulkIDs, nil +} + +func TestFeedbackRecordsService_DeleteFeedbackRecord_PublishesTenantAwareDeletedEvent(t *testing.T) { + ctx := context.Background() + recordID := uuid.Must(uuid.NewV7()) + tenantID := "org-123" + repo := &mockFeedbackRecordsRepo{record: &models.FeedbackRecord{ID: recordID, TenantID: tenantID}} + publisher := &capturePublisher{} + svc := NewFeedbackRecordsService(repo, nil, "", publisher, nil, "", 0) + + err := svc.DeleteFeedbackRecord(ctx, recordID) + if err != nil { + t.Fatalf("DeleteFeedbackRecord() error = %v", err) + } + + if repo.deletedID != recordID { + t.Fatalf("deletedID = %v, want %v", repo.deletedID, recordID) + } + + assertDeletedEventData(t, publisher, datatypes.FeedbackRecordDeleted, tenantID, []uuid.UUID{recordID}) +} + +func TestFeedbackRecordsService_BulkDeleteFeedbackRecords_PublishesTenantAwareDeletedEvent(t *testing.T) { + ctx := context.Background() + tenantID := "org-123" + ids := []uuid.UUID{uuid.Must(uuid.NewV7()), uuid.Must(uuid.NewV7())} + repo := &mockFeedbackRecordsRepo{bulkIDs: ids} + publisher := &capturePublisher{} + svc := NewFeedbackRecordsService(repo, nil, "", publisher, nil, "", 0) + + count, err := svc.BulkDeleteFeedbackRecords(ctx, "user-123", &tenantID) + if err != nil { + t.Fatalf("BulkDeleteFeedbackRecords() error = %v", err) + } + + if count != len(ids) { + t.Fatalf("count = %d, want %d", count, len(ids)) + } + + assertDeletedEventData(t, publisher, datatypes.FeedbackRecordDeleted, tenantID, ids) +} + +func TestFeedbackRecordsService_BulkDeleteFeedbackRecords_RequiresTenantID(t *testing.T) { + ctx := context.Background() + repo := &mockFeedbackRecordsRepo{bulkIDs: []uuid.UUID{uuid.Must(uuid.NewV7())}} + publisher := &capturePublisher{} + svc := NewFeedbackRecordsService(repo, nil, "", publisher, nil, "", 0) + + count, err := svc.BulkDeleteFeedbackRecords(ctx, "user-123", nil) + if !errors.Is(err, huberrors.ErrValidation) { + t.Fatalf("BulkDeleteFeedbackRecords() error = %v, want validation error", err) + } + + if count != 0 { + t.Fatalf("count = %d, want 0", count) + } + + if publisher.callCount != 0 { + t.Fatalf("published %d events, want 0", publisher.callCount) + } +} + +func assertDeletedEventData( + t *testing.T, publisher *capturePublisher, eventType datatypes.EventType, tenantID string, ids []uuid.UUID, +) { + t.Helper() + + if publisher.callCount != 1 || publisher.eventType != eventType { + t.Fatalf("published event = (%d, %s), want one %s", publisher.callCount, publisher.eventType, eventType) + } + + data, ok := publisher.data.(models.DeletedIDsEventData) + if !ok { + t.Fatalf("published data type = %T, want DeletedIDsEventData", publisher.data) + } + + if data.TenantID != tenantID { + t.Errorf("TenantID = %q, want %q", data.TenantID, tenantID) + } + + if len(data.IDs) != len(ids) { + t.Fatalf("IDs length = %d, want %d", len(data.IDs), len(ids)) + } + + for i := range ids { + if data.IDs[i] != ids[i] { + t.Errorf("IDs[%d] = %v, want %v", i, data.IDs[i], ids[i]) + } + } +} diff --git a/internal/service/tenant_validation.go b/internal/service/tenant_validation.go new file mode 100644 index 0000000..44af4fb --- /dev/null +++ b/internal/service/tenant_validation.go @@ -0,0 +1,20 @@ +package service + +import ( + "strings" + + "github.com/formbricks/hub/internal/huberrors" +) + +func normalizeRequiredTenantID(tenantID *string) (string, error) { + if tenantID == nil { + return "", huberrors.NewValidationError("tenant_id", "tenant_id is required") + } + + normalized := strings.TrimSpace(*tenantID) + if normalized == "" { + return "", huberrors.NewValidationError("tenant_id", "tenant_id is required and cannot be empty") + } + + return normalized, nil +} diff --git a/internal/service/webhook_payload.go b/internal/service/webhook_payload.go index 4526d0c..20df651 100644 --- a/internal/service/webhook_payload.go +++ b/internal/service/webhook_payload.go @@ -4,6 +4,8 @@ import ( "time" "github.com/google/uuid" + + "github.com/formbricks/hub/internal/models" ) // WebhookPayload represents a generic webhook payload structure for all event types. @@ -12,6 +14,55 @@ type WebhookPayload struct { ID uuid.UUID `json:"id"` // Unique event id (UUID v7) Type string `json:"type"` // Event type as string (e.g., "feedback_record.created", "webhook.created") Timestamp time.Time `json:"timestamp"` // Event creation timestamp + TenantID *string `json:"tenant_id,omitempty"` // Tenant boundary for the event Data any `json:"data"` // Event data (FeedbackRecord, Webhook, etc.) ChangedFields []string `json:"changed_fields,omitempty"` // Only for update events (optional) } + +// NewWebhookPayload builds the public webhook payload from internal dispatch args. +func NewWebhookPayload(args WebhookDispatchArgs) *WebhookPayload { + tenantID := clonePayloadTenantID(args.TenantID) + if tenantID == nil { + tenantID = TenantIDPointerFromEventData(args.Data) + } + + return &WebhookPayload{ + ID: args.EventID, + Type: args.EventType, + Timestamp: args.Timestamp, + TenantID: tenantID, + Data: publicWebhookData(args.Data), + ChangedFields: args.ChangedFields, + } +} + +func publicWebhookData(data any) any { + switch payload := data.(type) { + case models.DeletedIDsEventData: + return cloneUUIDs(payload.IDs) + case *models.DeletedIDsEventData: + if payload == nil { + return nil + } + + return cloneUUIDs(payload.IDs) + default: + return data + } +} + +func clonePayloadTenantID(tenantID *string) *string { + if tenantID == nil { + return nil + } + + return stringPointer(*tenantID) +} + +func cloneUUIDs(ids []uuid.UUID) []uuid.UUID { + if ids == nil { + return nil + } + + return append([]uuid.UUID(nil), ids...) +} diff --git a/internal/service/webhook_payload_test.go b/internal/service/webhook_payload_test.go new file mode 100644 index 0000000..0bb5d82 --- /dev/null +++ b/internal/service/webhook_payload_test.go @@ -0,0 +1,66 @@ +package service + +import ( + "testing" + "time" + + "github.com/google/uuid" + + "github.com/formbricks/hub/internal/models" +) + +func TestNewWebhookPayload_MapsDeletedIDsEventDataToPublicPayload(t *testing.T) { + tenantID := "org-123" + ids := []uuid.UUID{uuid.Must(uuid.NewV7()), uuid.Must(uuid.NewV7())} + args := WebhookDispatchArgs{ + EventID: uuid.Must(uuid.NewV7()), + EventType: "feedback_record.deleted", + Timestamp: time.Now(), + TenantID: &tenantID, + Data: models.DeletedIDsEventData{TenantID: tenantID, IDs: ids}, + WebhookID: uuid.Must(uuid.NewV7()), + } + + payload := NewWebhookPayload(args) + + if payload.TenantID == nil || *payload.TenantID != tenantID { + t.Fatalf("TenantID = %v, want %q", payload.TenantID, tenantID) + } + + gotIDs, ok := payload.Data.([]uuid.UUID) + if !ok { + t.Fatalf("Data type = %T, want []uuid.UUID", payload.Data) + } + + if len(gotIDs) != len(ids) { + t.Fatalf("Data length = %d, want %d", len(gotIDs), len(ids)) + } + + for i := range ids { + if gotIDs[i] != ids[i] { + t.Errorf("Data[%d] = %v, want %v", i, gotIDs[i], ids[i]) + } + } + + ids[0] = uuid.Must(uuid.NewV7()) + if gotIDs[0] == ids[0] { + t.Error("Data aliases internal deleted ID slice") + } +} + +func TestNewWebhookPayload_DerivesTenantFromLegacyArgsData(t *testing.T) { + tenantID := "org-123" + args := WebhookDispatchArgs{ + EventID: uuid.Must(uuid.NewV7()), + EventType: "feedback_record.created", + Timestamp: time.Now(), + Data: map[string]any{"tenant_id": tenantID}, + WebhookID: uuid.Must(uuid.NewV7()), + } + + payload := NewWebhookPayload(args) + + if payload.TenantID == nil || *payload.TenantID != tenantID { + t.Fatalf("TenantID = %v, want %q", payload.TenantID, tenantID) + } +} diff --git a/internal/service/webhook_provider.go b/internal/service/webhook_provider.go index 810e490..8e40167 100644 --- a/internal/service/webhook_provider.go +++ b/internal/service/webhook_provider.go @@ -19,9 +19,14 @@ type WebhookDispatchInserter interface { InsertMany(ctx context.Context, params []river.InsertManyParams) ([]*rivertype.JobInsertResult, error) } +// WebhookProviderRepository lists tenant-scoped webhooks eligible for event fan-out. +type WebhookProviderRepository interface { + ListEnabledForEventTypeAndTenant(ctx context.Context, eventType string, tenantID *string) ([]models.Webhook, error) +} + // WebhookProvider implements eventPublisher by enqueueing one River job per (event, webhook). type WebhookProvider struct { - repo WebhooksRepository + repo WebhookProviderRepository inserter WebhookDispatchInserter maxAttempts int maxFanOut int @@ -36,7 +41,7 @@ type WebhookProvider struct { // enqueueMaxRetries, enqueueInitialBackoff, enqueueMaxBackoff configure retries when InsertMany fails (transient River/DB errors). // metrics may be nil when metrics are disabled. func NewWebhookProvider( - inserter WebhookDispatchInserter, repo WebhooksRepository, + inserter WebhookDispatchInserter, repo WebhookProviderRepository, maxAttempts, maxFanOut int, enqueueMaxRetries int, enqueueInitialBackoff, enqueueMaxBackoff time.Duration, metrics observability.WebhookMetrics, @@ -54,16 +59,24 @@ func NewWebhookProvider( } // PublishEvent lists enabled webhooks for the event type and tenant, then enqueues one job per webhook. -// Tenant-scoped webhooks are only eligible when the event payload has the same tenant_id. +// Webhooks are only eligible when the event payload has the same tenant_id. func (p *WebhookProvider) PublishEvent(ctx context.Context, event Event) { tenantID := TenantIDPointerFromEventData(event.Data) if tenantID == nil { - slog.Debug("webhook provider: event has no tenant_id; tenant-scoped webhooks are not eligible", + if p.metrics != nil { + p.metrics.RecordProviderError(ctx, "missing_tenant_id") + } + + slog.Warn("webhook provider: event has no tenant_id; skipping webhook fan-out", "event_id", event.ID, "event_type", event.Type, ) + + return } + tenantIDValue := *tenantID + webhooks, err := p.repo.ListEnabledForEventTypeAndTenant(ctx, event.Type.String(), tenantID) if err != nil { if p.metrics != nil { @@ -73,6 +86,7 @@ func (p *WebhookProvider) PublishEvent(ctx context.Context, event Event) { slog.Error("failed to list enabled webhooks for event type", "event_id", event.ID, "event_type", event.Type, + "tenant_id", tenantIDValue, "error", err, ) @@ -84,6 +98,7 @@ func (p *WebhookProvider) PublishEvent(ctx context.Context, event Event) { slog.Warn("webhook provider: skipped tenant-mismatched webhooks returned by repository", "event_id", event.ID, "event_type", event.Type, + "tenant_id", tenantIDValue, "skipped", skipped, ) } @@ -147,6 +162,7 @@ func (p *WebhookProvider) PublishEvent(ctx context.Context, event Event) { slog.Error("failed to enqueue webhook jobs after retries", "event_id", event.ID, "event_type", event.Type, + "tenant_id", tenantIDValue, "error", insertErr, ) diff --git a/internal/service/webhook_provider_test.go b/internal/service/webhook_provider_test.go index 585b86a..73a2786 100644 --- a/internal/service/webhook_provider_test.go +++ b/internal/service/webhook_provider_test.go @@ -60,52 +60,18 @@ func (m *mockProviderRepo) ListEnabledForEventTypeAndTenant( return m.webhooks, nil } -// Stub other WebhooksRepository methods so mockProviderRepo can be used as WebhooksRepository. -func (m *mockProviderRepo) Create(_ context.Context, _ *models.CreateWebhookRequest) (*models.Webhook, error) { - return nil, errors.New("not implemented") -} - -func (m *mockProviderRepo) GetByID(_ context.Context, _ uuid.UUID) (*models.Webhook, error) { - return nil, errors.New("not implemented") -} - -func (m *mockProviderRepo) List(_ context.Context, _ *models.ListWebhooksFilters) ([]models.Webhook, bool, error) { - return nil, false, errors.New("not implemented") -} - -func (m *mockProviderRepo) ListAfterCursor( - _ context.Context, _ *models.ListWebhooksFilters, _ time.Time, _ uuid.UUID, -) ([]models.Webhook, bool, error) { - return nil, false, errors.New("not implemented") -} - -func (m *mockProviderRepo) Count(_ context.Context, _ *models.ListWebhooksFilters) (int64, error) { - return 0, errors.New("not implemented") -} - -func (m *mockProviderRepo) Update(_ context.Context, _ uuid.UUID, _ *models.UpdateWebhookRequest) (*models.Webhook, error) { - return nil, errors.New("not implemented") -} - -func (m *mockProviderRepo) Delete(_ context.Context, _ uuid.UUID) error { - return errors.New("not implemented") -} - -func (m *mockProviderRepo) ListEnabled(_ context.Context) ([]models.Webhook, error) { - return nil, errors.New("not implemented") -} - func TestWebhookProvider_PublishEvent(t *testing.T) { ctx := context.Background() eventID := uuid.Must(uuid.NewV7()) eventType := datatypes.FeedbackRecordCreated wh1 := uuid.Must(uuid.NewV7()) wh2 := uuid.Must(uuid.NewV7()) + tenantID := "org-123" t.Run("inserts one job per webhook via InsertMany with correct opts", func(t *testing.T) { inserter := &mockWebhookInserter{} repo := &mockProviderRepo{ - webhooks: []models.Webhook{{ID: wh1}, {ID: wh2}}, + webhooks: []models.Webhook{{ID: wh1, TenantID: &tenantID}, {ID: wh2, TenantID: &tenantID}}, } provider := NewWebhookProvider(inserter, repo, 3, 500, 0, 0, 0, nil) @@ -113,7 +79,7 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { ID: eventID, Type: eventType, Timestamp: time.Now(), - Data: map[string]string{"id": "123"}, + Data: map[string]string{"id": "123", "tenant_id": tenantID}, } provider.PublishEvent(ctx, event) @@ -122,8 +88,8 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { t.Errorf("eventType = %q, want %q", repo.eventType, eventType.String()) } - if repo.tenantID != nil { - t.Errorf("tenantID = %q, want nil for tenant-less event", *repo.tenantID) + if repo.tenantID == nil || *repo.tenantID != tenantID { + t.Errorf("tenantID = %v, want %q", repo.tenantID, tenantID) } if n := len(inserter.insertManyCalls); n != 1 { @@ -154,8 +120,8 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { t.Errorf("param %d WebhookID = %v, want %v", i, args.WebhookID, wantID) } - if args.TenantID != nil { - t.Errorf("param %d TenantID = %q, want nil", i, *args.TenantID) + if args.TenantID == nil || *args.TenantID != tenantID { + t.Errorf("param %d TenantID = %v, want %q", i, args.TenantID, tenantID) } if p.InsertOpts == nil || p.InsertOpts.MaxAttempts != 3 { @@ -172,7 +138,7 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { inserter := &mockWebhookInserter{} repo := &mockProviderRepo{webhooks: nil} provider := NewWebhookProvider(inserter, repo, 3, 500, 0, 0, 0, nil) - event := Event{ID: eventID, Type: eventType, Timestamp: time.Now(), Data: nil} + event := Event{ID: eventID, Type: eventType, Timestamp: time.Now(), Data: map[string]string{"tenant_id": tenantID}} provider.PublishEvent(ctx, event) if len(inserter.insertManyCalls) != 0 { @@ -184,7 +150,7 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { inserter := &mockWebhookInserter{} repo := &mockProviderRepo{err: errors.New("db error")} provider := NewWebhookProvider(inserter, repo, 3, 500, 0, 0, 0, nil) - event := Event{ID: eventID, Type: eventType, Timestamp: time.Now(), Data: nil} + event := Event{ID: eventID, Type: eventType, Timestamp: time.Now(), Data: map[string]string{"tenant_id": tenantID}} provider.PublishEvent(ctx, event) if len(inserter.insertManyCalls) != 0 { @@ -194,9 +160,11 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { t.Run("when InsertMany returns error, provider logs and returns", func(t *testing.T) { inserter := &mockWebhookInserter{insertManyErr: errors.New("river error")} - repo := &mockProviderRepo{webhooks: []models.Webhook{{ID: wh1}, {ID: wh2}}} + repo := &mockProviderRepo{ + webhooks: []models.Webhook{{ID: wh1, TenantID: &tenantID}, {ID: wh2, TenantID: &tenantID}}, + } provider := NewWebhookProvider(inserter, repo, 5, 500, 0, 0, 0, nil) - event := Event{ID: eventID, Type: eventType, Timestamp: time.Now(), Data: nil} + event := Event{ID: eventID, Type: eventType, Timestamp: time.Now(), Data: map[string]string{"tenant_id": tenantID}} provider.PublishEvent(ctx, event) // InsertMany was still called once (batch fails as a whole). if len(inserter.insertManyCalls) != 1 { @@ -217,12 +185,12 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { webhooks := make([]models.Webhook, 501) for i := range webhooks { - webhooks[i] = models.Webhook{ID: uuid.Must(uuid.NewV7())} + webhooks[i] = models.Webhook{ID: uuid.Must(uuid.NewV7()), TenantID: &tenantID} } repo := &mockProviderRepo{webhooks: webhooks} provider := NewWebhookProvider(inserter, repo, 3, 500, 0, 0, 0, nil) - event := Event{ID: eventID, Type: eventType, Timestamp: time.Now(), Data: nil} + event := Event{ID: eventID, Type: eventType, Timestamp: time.Now(), Data: map[string]string{"tenant_id": tenantID}} provider.PublishEvent(ctx, event) if len(inserter.insertManyCalls) != 2 { @@ -268,8 +236,8 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { } params := inserter.insertManyCalls[0] - if len(params) != 2 { - t.Fatalf("InsertMany params length = %d, want 2", len(params)) + if len(params) != 1 { + t.Fatalf("InsertMany params length = %d, want 1", len(params)) } gotWebhookIDs := map[uuid.UUID]bool{} @@ -287,18 +255,17 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { gotWebhookIDs[args.WebhookID] = true } - if !gotWebhookIDs[wh1] || !gotWebhookIDs[wh2] { - t.Errorf("enqueued webhook IDs = %v, want global and matching tenant webhooks", gotWebhookIDs) + if !gotWebhookIDs[wh2] || gotWebhookIDs[wh1] { + t.Errorf("enqueued webhook IDs = %v, want only matching tenant webhook", gotWebhookIDs) } }) - t.Run("tenant-less events only enqueue global webhooks", func(t *testing.T) { - tenantA := "org-123" + t.Run("tenant-less events do not query or enqueue webhooks", func(t *testing.T) { inserter := &mockWebhookInserter{} repo := &mockProviderRepo{ webhooks: []models.Webhook{ {ID: wh1}, - {ID: wh2, TenantID: &tenantA}, + {ID: wh2, TenantID: &tenantID}, }, } provider := NewWebhookProvider(inserter, repo, 3, 500, 0, 0, 0, nil) @@ -306,30 +273,12 @@ func TestWebhookProvider_PublishEvent(t *testing.T) { provider.PublishEvent(ctx, event) - if repo.tenantID != nil { - t.Fatalf("tenantID = %q, want nil", *repo.tenantID) + if repo.listCallCount != 0 { + t.Fatalf("ListEnabledForEventTypeAndTenant called %d times, want 0", repo.listCallCount) } - if len(inserter.insertManyCalls) != 1 { - t.Fatalf("InsertMany called %d times, want 1", len(inserter.insertManyCalls)) - } - - params := inserter.insertManyCalls[0] - if len(params) != 1 { - t.Fatalf("InsertMany params length = %d, want 1", len(params)) - } - - args, ok := params[0].Args.(WebhookDispatchArgs) - if !ok { - t.Fatalf("Args type = %T, want WebhookDispatchArgs", params[0].Args) - } - - if args.WebhookID != wh1 { - t.Errorf("WebhookID = %v, want %v", args.WebhookID, wh1) - } - - if args.TenantID != nil { - t.Errorf("TenantID = %q, want nil", *args.TenantID) + if len(inserter.insertManyCalls) != 0 { + t.Fatalf("InsertMany called %d times, want 0", len(inserter.insertManyCalls)) } }) } diff --git a/internal/service/webhook_sender.go b/internal/service/webhook_sender.go index ef5ef6d..66224f3 100644 --- a/internal/service/webhook_sender.go +++ b/internal/service/webhook_sender.go @@ -12,6 +12,7 @@ import ( "strconv" "time" + "github.com/google/uuid" standardwebhooks "github.com/standard-webhooks/standard-webhooks/libraries/go" "github.com/formbricks/hub/internal/models" @@ -29,9 +30,14 @@ type WebhookSender interface { Send(ctx context.Context, webhook *models.Webhook, payload *WebhookPayload) error } +// WebhookSenderRepository persists webhook state changes caused by delivery. +type WebhookSenderRepository interface { + Update(ctx context.Context, id uuid.UUID, req *models.UpdateWebhookRequest) (*models.Webhook, error) +} + // WebhookSenderImpl implements WebhookSender with Standard Webhooks conformance. type WebhookSenderImpl struct { - repo WebhooksRepository + repo WebhookSenderRepository httpClient *http.Client metrics observability.WebhookMetrics urlHostBlacklist map[string]struct{} @@ -44,7 +50,7 @@ type WebhookSenderImpl struct { // metrics may be nil when metrics are disabled. // If httpClient is non-nil, it is used as-is (e.g. for tests that hit loopback); otherwise a secured client is built. func NewWebhookSenderImpl( - repo WebhooksRepository, metrics observability.WebhookMetrics, urlHostBlacklist map[string]struct{}, + repo WebhookSenderRepository, metrics observability.WebhookMetrics, urlHostBlacklist map[string]struct{}, httpTimeout time.Duration, httpClient *http.Client, ) *WebhookSenderImpl { if httpClient == nil { diff --git a/internal/service/webhook_sender_test.go b/internal/service/webhook_sender_test.go index ee67974..1e57613 100644 --- a/internal/service/webhook_sender_test.go +++ b/internal/service/webhook_sender_test.go @@ -24,42 +24,6 @@ func (m *mockSenderRepo) Update(_ context.Context, _ uuid.UUID, _ *models.Update return nil, m.updateErr } -func (m *mockSenderRepo) Create(_ context.Context, _ *models.CreateWebhookRequest) (*models.Webhook, error) { - return nil, nil -} - -func (m *mockSenderRepo) GetByID(_ context.Context, _ uuid.UUID) (*models.Webhook, error) { - return nil, nil -} - -func (m *mockSenderRepo) List(_ context.Context, _ *models.ListWebhooksFilters) ([]models.Webhook, bool, error) { - return nil, false, nil -} - -func (m *mockSenderRepo) ListAfterCursor( - _ context.Context, _ *models.ListWebhooksFilters, _ time.Time, _ uuid.UUID, -) ([]models.Webhook, bool, error) { - return nil, false, nil -} - -func (m *mockSenderRepo) Count(_ context.Context, _ *models.ListWebhooksFilters) (int64, error) { - return 0, nil -} - -func (m *mockSenderRepo) Delete(_ context.Context, _ uuid.UUID) error { - return nil -} - -func (m *mockSenderRepo) ListEnabled(_ context.Context) ([]models.Webhook, error) { - return nil, nil -} - -func (m *mockSenderRepo) ListEnabledForEventTypeAndTenant( - _ context.Context, _ string, _ *string, -) ([]models.Webhook, error) { - return nil, nil -} - func TestWebhookSenderImpl_Send(t *testing.T) { ctx := context.Background() webhookID := uuid.Must(uuid.NewV7()) diff --git a/internal/service/webhook_tenant.go b/internal/service/webhook_tenant.go index 9ac21c2..00b88b9 100644 --- a/internal/service/webhook_tenant.go +++ b/internal/service/webhook_tenant.go @@ -33,6 +33,14 @@ func TenantIDFromEventData(data any) (string, bool) { return tenantIDFromPointer(payload.TenantID) case models.WebhookPublic: return tenantIDFromPointer(payload.TenantID) + case *models.DeletedIDsEventData: + if payload == nil { + return "", false + } + + return tenantIDFromString(payload.TenantID) + case models.DeletedIDsEventData: + return tenantIDFromString(payload.TenantID) case map[string]any: return tenantIDFromMapValue(payload["tenant_id"]) case map[string]string: @@ -60,11 +68,7 @@ func WebhookMatchesTenant(webhook *models.Webhook, tenantID *string) bool { return false } - if webhook.TenantID == nil { - return true - } - - if tenantID == nil { + if webhook.TenantID == nil || tenantID == nil { return false } diff --git a/internal/service/webhook_tenant_test.go b/internal/service/webhook_tenant_test.go index 8c3aa10..b5ef8d0 100644 --- a/internal/service/webhook_tenant_test.go +++ b/internal/service/webhook_tenant_test.go @@ -34,6 +34,12 @@ func TestTenantIDFromEventData(t *testing.T) { want: tenantID, ok: true, }, + { + name: "deleted IDs event data", + data: models.DeletedIDsEventData{TenantID: tenantID}, + want: tenantID, + ok: true, + }, { name: "map any", data: map[string]any{"tenant_id": tenantID}, @@ -96,8 +102,8 @@ func TestWebhookMatchesTenant(t *testing.T) { tenantID *string want bool }{ - {name: "global webhook matches tenant event", webhook: &models.Webhook{}, tenantID: &tenantID, want: true}, - {name: "global webhook matches tenant-less event", webhook: &models.Webhook{}, tenantID: nil, want: true}, + {name: "webhook without tenant rejects tenant event", webhook: &models.Webhook{}, tenantID: &tenantID, want: false}, + {name: "webhook without tenant rejects tenant-less event", webhook: &models.Webhook{}, tenantID: nil, want: false}, {name: "scoped webhook matches same tenant", webhook: &models.Webhook{TenantID: &tenantID}, tenantID: &tenantID, want: true}, {name: "scoped webhook rejects different tenant", webhook: &models.Webhook{TenantID: &tenantID}, tenantID: &otherTenantID, want: false}, {name: "scoped webhook rejects tenant-less event", webhook: &models.Webhook{TenantID: &tenantID}, tenantID: nil, want: false}, diff --git a/internal/service/webhooks_service.go b/internal/service/webhooks_service.go index b5c4d0a..4549bd5 100644 --- a/internal/service/webhooks_service.go +++ b/internal/service/webhooks_service.go @@ -5,6 +5,7 @@ import ( "crypto/rand" "encoding/base64" "fmt" + "log/slog" "net" "net/netip" "net/url" @@ -32,8 +33,6 @@ type WebhooksRepository interface { Count(ctx context.Context, filters *models.ListWebhooksFilters) (int64, error) Update(ctx context.Context, id uuid.UUID, req *models.UpdateWebhookRequest) (*models.Webhook, error) Delete(ctx context.Context, id uuid.UUID) error - ListEnabled(ctx context.Context) ([]models.Webhook, error) - ListEnabledForEventTypeAndTenant(ctx context.Context, eventType string, tenantID *string) ([]models.Webhook, error) } // WebhooksService handles business logic for webhooks. @@ -59,6 +58,10 @@ func NewWebhooksService( // CreateWebhook creates a new webhook. func (s *WebhooksService) CreateWebhook(ctx context.Context, req *models.CreateWebhookRequest) (*models.Webhook, error) { + if err := normalizeRequiredWebhookTenantID(req.TenantID); err != nil { + return nil, err + } + count, err := s.repo.Count(ctx, &models.ListWebhooksFilters{}) if err != nil { return nil, fmt.Errorf("count webhooks: %w", err) @@ -347,6 +350,10 @@ func (s *WebhooksService) ListWebhooks(ctx context.Context, filters *models.List // UpdateWebhook updates an existing webhook. func (s *WebhooksService) UpdateWebhook(ctx context.Context, id uuid.UUID, req *models.UpdateWebhookRequest) (*models.Webhook, error) { + if err := normalizeOptionalWebhookTenantID(req.TenantID); err != nil { + return nil, err + } + if req.URL != nil { if err := validateWebhookURLHost(ctx, *req.URL, s.urlHostBlacklist); err != nil { return nil, err @@ -369,14 +376,58 @@ func (s *WebhooksService) UpdateWebhook(ctx context.Context, id uuid.UUID, req * return webhook, nil } +func normalizeRequiredWebhookTenantID(tenantID *string) error { + normalized, err := normalizeRequiredTenantID(tenantID) + if err != nil { + return err + } + + *tenantID = normalized + + return nil +} + +func normalizeOptionalWebhookTenantID(tenantID *string) error { + if tenantID == nil { + return nil + } + + return normalizeWebhookTenantID(tenantID) +} + +func normalizeWebhookTenantID(tenantID *string) error { + normalized, err := normalizeRequiredTenantID(tenantID) + if err != nil { + return err + } + + *tenantID = normalized + + return nil +} + // DeleteWebhook deletes a webhook by ID. -// Publishes WebhookDeleted with data = [id] (array of deleted IDs) for consistency with feedback record deletes. +// Publishes WebhookDeleted with tenant-aware deleted IDs. func (s *WebhooksService) DeleteWebhook(ctx context.Context, id uuid.UUID) error { + webhook, err := s.repo.GetByID(ctx, id) + if err != nil { + return fmt.Errorf("get webhook before delete: %w", err) + } + if err := s.repo.Delete(ctx, id); err != nil { return fmt.Errorf("delete webhook: %w", err) } - s.publisher.PublishEvent(ctx, datatypes.WebhookDeleted, []uuid.UUID{id}) + if webhook.TenantID == nil { + slog.Warn("webhook delete: tenant_id missing, skipping webhook event", "webhook_id", id) + + return nil + } + + s.publisher.PublishEvent(ctx, datatypes.WebhookDeleted, models.DeletedIDsEventData{ + TenantID: *webhook.TenantID, + IDs: []uuid.UUID{id}, + }) return nil } diff --git a/internal/service/webhooks_service_test.go b/internal/service/webhooks_service_test.go index b3addc2..8269424 100644 --- a/internal/service/webhooks_service_test.go +++ b/internal/service/webhooks_service_test.go @@ -15,7 +15,9 @@ import ( ) type mockWebhooksRepo struct { - count int64 + count int64 + webhook *models.Webhook + deletedID uuid.UUID } func (m *mockWebhooksRepo) Create(_ context.Context, _ *models.CreateWebhookRequest) (*models.Webhook, error) { @@ -23,6 +25,10 @@ func (m *mockWebhooksRepo) Create(_ context.Context, _ *models.CreateWebhookRequ } func (m *mockWebhooksRepo) GetByID(_ context.Context, _ uuid.UUID) (*models.Webhook, error) { + if m.webhook != nil { + return m.webhook, nil + } + return nil, nil } @@ -44,18 +50,10 @@ func (m *mockWebhooksRepo) Update(_ context.Context, _ uuid.UUID, _ *models.Upda return nil, nil } -func (m *mockWebhooksRepo) Delete(_ context.Context, _ uuid.UUID) error { - return nil -} - -func (m *mockWebhooksRepo) ListEnabled(_ context.Context) ([]models.Webhook, error) { - return nil, nil -} +func (m *mockWebhooksRepo) Delete(_ context.Context, id uuid.UUID) error { + m.deletedID = id -func (m *mockWebhooksRepo) ListEnabledForEventTypeAndTenant( - _ context.Context, _ string, _ *string, -) ([]models.Webhook, error) { - return nil, nil + return nil } type noopPublisher struct{} @@ -65,13 +63,37 @@ func (noopPublisher) PublishEvent(_ context.Context, _ datatypes.EventType, _ an func (noopPublisher) PublishEventWithChangedFields(_ context.Context, _ datatypes.EventType, _ any, _ []string) { } +type capturePublisher struct { + eventType datatypes.EventType + data any + changedFields []string + callCount int +} + +func (p *capturePublisher) PublishEvent(_ context.Context, eventType datatypes.EventType, data any) { + p.eventType = eventType + p.data = data + p.callCount++ +} + +func (p *capturePublisher) PublishEventWithChangedFields( + _ context.Context, eventType datatypes.EventType, data any, changedFields []string, +) { + p.eventType = eventType + p.data = data + p.changedFields = changedFields + p.callCount++ +} + func TestWebhooksService_CreateWebhook_InvalidSigningKey(t *testing.T) { ctx := context.Background() svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, nil) + tenantID := "org-123" req := &models.CreateWebhookRequest{ URL: "https://example.com/webhook", SigningKey: "not-valid", + TenantID: &tenantID, EventTypes: []datatypes.EventType{datatypes.FeedbackRecordCreated}, } @@ -109,6 +131,7 @@ func TestWebhooksService_CreateWebhook_RejectsSSRFHosts(t *testing.T) { ctx := context.Background() svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, ssrfBlacklist) validKey := "whsec_" + "abcdefghijklmnopqrstuvwxyz123456" + tenantID := "org-123" tests := []struct { name string @@ -127,6 +150,7 @@ func TestWebhooksService_CreateWebhook_RejectsSSRFHosts(t *testing.T) { req := &models.CreateWebhookRequest{ URL: tt.url, SigningKey: validKey, + TenantID: &tenantID, EventTypes: []datatypes.EventType{datatypes.FeedbackRecordCreated}, } @@ -143,6 +167,71 @@ func TestWebhooksService_CreateWebhook_RejectsSSRFHosts(t *testing.T) { } } +func TestWebhooksService_CreateWebhook_RequiresTenantID(t *testing.T) { + ctx := context.Background() + svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, nil) + + req := &models.CreateWebhookRequest{ + URL: "https://example.com/webhook", + SigningKey: "whsec_" + "abcdefghijklmnopqrstuvwxyz123456", + EventTypes: []datatypes.EventType{datatypes.FeedbackRecordCreated}, + } + + _, err := svc.CreateWebhook(ctx, req) + if !errors.Is(err, huberrors.ErrValidation) { + t.Fatalf("expected ErrValidation, got %v", err) + } +} + +func TestWebhooksService_UpdateWebhook_RejectsEmptyTenantID(t *testing.T) { + ctx := context.Background() + svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, nil) + id := uuid.Must(uuid.NewV7()) + tenantID := " " + + req := &models.UpdateWebhookRequest{TenantID: &tenantID} + + _, err := svc.UpdateWebhook(ctx, id, req) + if !errors.Is(err, huberrors.ErrValidation) { + t.Fatalf("expected ErrValidation, got %v", err) + } +} + +func TestWebhooksService_DeleteWebhook_PublishesTenantAwareDeletedEvent(t *testing.T) { + ctx := context.Background() + webhookID := uuid.Must(uuid.NewV7()) + tenantID := "org-123" + repo := &mockWebhooksRepo{webhook: &models.Webhook{ID: webhookID, TenantID: &tenantID}} + publisher := &capturePublisher{} + svc := NewWebhooksService(repo, publisher, 10, nil) + + err := svc.DeleteWebhook(ctx, webhookID) + if err != nil { + t.Fatalf("DeleteWebhook() error = %v", err) + } + + if repo.deletedID != webhookID { + t.Fatalf("deletedID = %v, want %v", repo.deletedID, webhookID) + } + + if publisher.callCount != 1 || publisher.eventType != datatypes.WebhookDeleted { + t.Fatalf("published event = (%d, %s), want one webhook.deleted", publisher.callCount, publisher.eventType) + } + + data, ok := publisher.data.(models.DeletedIDsEventData) + if !ok { + t.Fatalf("published data type = %T, want DeletedIDsEventData", publisher.data) + } + + if data.TenantID != tenantID { + t.Errorf("TenantID = %q, want %q", data.TenantID, tenantID) + } + + if len(data.IDs) != 1 || data.IDs[0] != webhookID { + t.Errorf("IDs = %v, want [%v]", data.IDs, webhookID) + } +} + func TestWebhooksService_UpdateWebhook_RejectsSSRFHosts(t *testing.T) { ctx := context.Background() svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, ssrfBlacklist) diff --git a/internal/workers/webhook_dispatch.go b/internal/workers/webhook_dispatch.go index 2e0f871..1fa7072 100644 --- a/internal/workers/webhook_dispatch.go +++ b/internal/workers/webhook_dispatch.go @@ -84,9 +84,27 @@ func (w *WebhookDispatchWorker) Work(ctx context.Context, job *river.Job[service return nil } - tenantID := args.TenantID + jobTenantID := args.TenantID + dataTenantID := service.TenantIDPointerFromEventData(args.Data) + + if jobTenantID != nil && dataTenantID != nil && *jobTenantID != *dataTenantID { + if w.metrics != nil { + w.metrics.RecordDispatchError(ctx, "tenant_mismatch") + } + + slog.Error("webhook dispatch: job tenant_id conflicts with payload tenant_id, skipping delivery", + "event_id", args.EventID, + "webhook_id", args.WebhookID, + "job_tenant_id", *jobTenantID, + "payload_tenant_id", *dataTenantID, + ) + + return nil + } + + tenantID := jobTenantID if tenantID == nil { - tenantID = service.TenantIDPointerFromEventData(args.Data) + tenantID = dataTenantID } if !service.WebhookMatchesTenant(webhook, tenantID) { @@ -114,7 +132,7 @@ func (w *WebhookDispatchWorker) Work(ctx context.Context, job *river.Job[service return nil } - payload := argsToPayload(args) + payload := service.NewWebhookPayload(args) err = w.sender.Send(ctx, webhook, payload) if err == nil { @@ -176,14 +194,3 @@ func (w *WebhookDispatchWorker) Work(ctx context.Context, job *river.Job[service return fmt.Errorf("webhook send: %w", err) } - -// argsToPayload builds a WebhookPayload from job args. -func argsToPayload(args service.WebhookDispatchArgs) *service.WebhookPayload { - return &service.WebhookPayload{ - ID: args.EventID, - Type: args.EventType, - Timestamp: args.Timestamp, - Data: args.Data, - ChangedFields: args.ChangedFields, - } -} diff --git a/internal/workers/webhook_dispatch_test.go b/internal/workers/webhook_dispatch_test.go index 91b1eff..0b20701 100644 --- a/internal/workers/webhook_dispatch_test.go +++ b/internal/workers/webhook_dispatch_test.go @@ -31,12 +31,14 @@ func (m *mockDispatchRepo) Update(_ context.Context, _ uuid.UUID, req *models.Up } type mockSender struct { - err error - calls int + err error + calls int + payloads []*service.WebhookPayload } -func (m *mockSender) Send(_ context.Context, _ *models.Webhook, _ *service.WebhookPayload) error { +func (m *mockSender) Send(_ context.Context, _ *models.Webhook, payload *service.WebhookPayload) error { m.calls++ + m.payloads = append(m.payloads, payload) return m.err } @@ -45,11 +47,13 @@ func TestWebhookDispatchWorker_Work(t *testing.T) { ctx := context.Background() eventID := uuid.Must(uuid.NewV7()) webhookID := uuid.Must(uuid.NewV7()) + tenantID := "org-123" args := service.WebhookDispatchArgs{ EventID: eventID, EventType: "feedback_record.created", Timestamp: time.Now(), Data: nil, + TenantID: &tenantID, WebhookID: webhookID, } @@ -78,7 +82,9 @@ func TestWebhookDispatchWorker_Work(t *testing.T) { }) t.Run("returns nil on send success", func(t *testing.T) { - repo := &mockDispatchRepo{webhook: &models.Webhook{ID: webhookID, Enabled: true, URL: "http://x", SigningKey: "sk"}} + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ID: webhookID, Enabled: true, URL: "http://x", SigningKey: "sk", TenantID: &tenantID}, + } sender := &mockSender{} worker := NewWebhookDispatchWorker(repo, sender, 15*time.Second, nil) job := &river.Job[service.WebhookDispatchArgs]{JobRow: &rivertype.JobRow{}, Args: args} @@ -98,7 +104,6 @@ func TestWebhookDispatchWorker_Work(t *testing.T) { }) t.Run("returns nil without send when scoped webhook tenant mismatches job tenant", func(t *testing.T) { - webhookTenant := "org-123" eventTenant := "org-other" repo := &mockDispatchRepo{ webhook: &models.Webhook{ @@ -106,7 +111,7 @@ func TestWebhookDispatchWorker_Work(t *testing.T) { Enabled: true, URL: "http://x", SigningKey: "sk", - TenantID: &webhookTenant, + TenantID: &tenantID, }, } sender := &mockSender{} @@ -157,8 +162,35 @@ func TestWebhookDispatchWorker_Work(t *testing.T) { } }) + t.Run("returns nil without send when job tenant conflicts with payload tenant", func(t *testing.T) { + payloadTenant := "org-other" + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ + ID: webhookID, + Enabled: true, + URL: "http://x", + SigningKey: "sk", + TenantID: &tenantID, + }, + } + sender := &mockSender{} + worker := NewWebhookDispatchWorker(repo, sender, 15*time.Second, nil) + scopedArgs := args + scopedArgs.TenantID = &tenantID + scopedArgs.Data = map[string]any{"tenant_id": payloadTenant} + job := &river.Job[service.WebhookDispatchArgs]{JobRow: &rivertype.JobRow{}, Args: scopedArgs} + + err := worker.Work(ctx, job) + if err != nil { + t.Errorf("Work() error = %v, want nil", err) + } + + if sender.calls != 0 { + t.Errorf("Send called %d times, want 0", sender.calls) + } + }) + t.Run("sends when scoped webhook tenant matches job tenant", func(t *testing.T) { - tenantID := "org-123" repo := &mockDispatchRepo{ webhook: &models.Webhook{ ID: webhookID, @@ -184,8 +216,42 @@ func TestWebhookDispatchWorker_Work(t *testing.T) { } }) + t.Run("sends legacy job and includes derived tenant in payload", func(t *testing.T) { + webhookTenant := "org-123" + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ + ID: webhookID, + Enabled: true, + URL: "http://x", + SigningKey: "sk", + TenantID: &webhookTenant, + }, + } + sender := &mockSender{} + worker := NewWebhookDispatchWorker(repo, sender, 15*time.Second, nil) + scopedArgs := args + scopedArgs.Data = map[string]any{"tenant_id": webhookTenant} + scopedArgs.TenantID = nil + job := &river.Job[service.WebhookDispatchArgs]{JobRow: &rivertype.JobRow{}, Args: scopedArgs} + + err := worker.Work(ctx, job) + if err != nil { + t.Errorf("Work() error = %v, want nil", err) + } + + if sender.calls != 1 { + t.Fatalf("Send called %d times, want 1", sender.calls) + } + + if sender.payloads[0].TenantID == nil || *sender.payloads[0].TenantID != webhookTenant { + t.Errorf("payload tenant_id = %v, want %q", sender.payloads[0].TenantID, webhookTenant) + } + }) + t.Run("returns error and does not update when send fails and attempt < max", func(t *testing.T) { - repo := &mockDispatchRepo{webhook: &models.Webhook{ID: webhookID, Enabled: true, URL: "http://x", SigningKey: "sk"}} + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ID: webhookID, Enabled: true, URL: "http://x", SigningKey: "sk", TenantID: &tenantID}, + } sender := &mockSender{err: errors.New("network error")} worker := NewWebhookDispatchWorker(repo, sender, 15*time.Second, nil) job := &river.Job[service.WebhookDispatchArgs]{ @@ -204,7 +270,9 @@ func TestWebhookDispatchWorker_Work(t *testing.T) { }) t.Run("updates webhook and returns error when send fails on last attempt", func(t *testing.T) { - repo := &mockDispatchRepo{webhook: &models.Webhook{ID: webhookID, Enabled: true, URL: "http://x", SigningKey: "sk"}} + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ID: webhookID, Enabled: true, URL: "http://x", SigningKey: "sk", TenantID: &tenantID}, + } sender := &mockSender{err: errors.New("final failure")} worker := NewWebhookDispatchWorker(repo, sender, 15*time.Second, nil) job := &river.Job[service.WebhookDispatchArgs]{ diff --git a/migrations/008_require_webhook_tenant_id.sql b/migrations/008_require_webhook_tenant_id.sql new file mode 100644 index 0000000..576fea5 --- /dev/null +++ b/migrations/008_require_webhook_tenant_id.sql @@ -0,0 +1,18 @@ +-- +goose Up +-- Webhooks are tenant-owned dispatch configuration. Disable legacy global rows and +-- prevent new NULL or empty tenant IDs without blocking this migration on old data. +UPDATE webhooks +SET + enabled = false, + disabled_reason = COALESCE(disabled_reason, 'Disabled by migration: tenant_id is required for webhook isolation'), + disabled_at = COALESCE(disabled_at, NOW()), + updated_at = NOW() +WHERE tenant_id IS NULL OR btrim(tenant_id) = ''; + +ALTER TABLE webhooks + ADD CONSTRAINT webhooks_tenant_id_required + CHECK (tenant_id IS NOT NULL AND btrim(tenant_id) <> '') NOT VALID; + +-- +goose Down +ALTER TABLE webhooks + DROP CONSTRAINT IF EXISTS webhooks_tenant_id_required; diff --git a/openapi.yaml b/openapi.yaml index 19051e3..653bbc7 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -381,7 +381,7 @@ paths: tags: - Feedback Records summary: Bulk delete feedback records by user ID - description: Permanently deletes all feedback record data points matching the specified user_id. This endpoint supports GDPR Article 17 (Right to Erasure) requests. + description: Permanently deletes all feedback record data points matching the specified user_id within the required tenant_id. This endpoint supports GDPR Article 17 (Right to Erasure) requests. operationId: bulk-delete-feedback-records parameters: - name: user_id @@ -396,10 +396,12 @@ paths: example: "user-abc-123" - name: tenant_id in: query - description: Filter by tenant ID (optional, for multi-tenant deployments). NULL bytes not allowed. + description: Tenant ID to delete within (required for isolation). NULL bytes not allowed. + required: true schema: type: string - description: Filter by tenant ID (optional, for multi-tenant deployments). NULL bytes not allowed. + description: Tenant ID to delete within (required for isolation). NULL bytes not allowed. + minLength: 1 pattern: '^[^\x00]*$' example: "org-123" responses: @@ -837,6 +839,7 @@ paths: Creates a new webhook endpoint. When events occur (e.g. feedback_record.created), the Hub POSTs a signed payload to the webhook URL. If signing_key is omitted, a key is auto-generated (Standard Webhooks format, whsec_...). + tenant_id is required; webhooks only receive events from that exact tenant. See WebhookDeliveryPayload for the payload structure sent to your URL. operationId: create-webhook requestBody: @@ -849,6 +852,7 @@ paths: summary: Subscribe to feedback events value: url: "https://example.com/hub-events" + tenant_id: "org-123" event_types: - "feedback_record.created" - "feedback_record.updated" @@ -1430,7 +1434,8 @@ components: description: Whether the webhook is active (default true) tenant_id: type: string - description: Tenant/organization identifier. NULL bytes not allowed. + description: Tenant/organization identifier. Required for webhook isolation; NULL bytes not allowed. + minLength: 1 maxLength: 255 pattern: '^[^\x00]*$' example: "org-123" @@ -1443,6 +1448,7 @@ components: $ref: '#/components/schemas/WebhookEventType' required: - url + - tenant_id ListWebhooksOutputBody: type: object additionalProperties: false @@ -1492,8 +1498,9 @@ components: type: boolean description: Enable or disable the webhook tenant_id: - type: [string, "null"] - description: Omit or send null to leave unchanged. Send empty string to clear (store as null). + type: string + description: Omit to leave unchanged. Empty strings are rejected; webhooks cannot be global. + minLength: 1 maxLength: 255 pattern: '^[^\x00]*$' example: "org-123" @@ -1544,6 +1551,7 @@ components: - id - url - enabled + - tenant_id - created_at - updated_at WebhookData: @@ -1591,6 +1599,7 @@ components: - url - signing_key - enabled + - tenant_id - created_at - updated_at WebhookEventType: @@ -1622,6 +1631,10 @@ components: The webhook-id header is a stable identifier per event: the same value is sent for every delivery attempt (all endpoints and all retries) for that event. Use it as an idempotency key (e.g. store seen IDs for a short window and skip duplicate processing). additionalProperties: false properties: + id: + type: string + format: uuid + description: Stable event ID. Matches the webhook-id header and can be used as an idempotency key. type: type: string description: Event type that occurred @@ -1636,13 +1649,16 @@ components: type: string format: date-time description: When the event was published (ISO 8601) + tenant_id: + type: string + description: Tenant/organization identifier for the event. data: description: | Event payload. Shape depends on event type: - feedback_record.created / feedback_record.updated: FeedbackRecordData (object). - feedback_record.deleted: DeletedIdsPayload (array of deleted feedback record IDs). - webhook.created / webhook.updated: WebhookData (object). - - webhook.deleted: DeletedIdsPayload (array of deleted webhook IDs; one element for single delete). + - webhook.deleted: DeletedIdsPayload (array of deleted webhook IDs; one ID for single delete). oneOf: - $ref: '#/components/schemas/FeedbackRecordData' - $ref: '#/components/schemas/WebhookData' @@ -1653,8 +1669,10 @@ components: items: type: string required: + - id - type - timestamp + - tenant_id - data UpdateFeedbackRecordInputBody: type: object diff --git a/tests/integration_test.go b/tests/integration_test.go index 6db9ef3..af78c5f 100644 --- a/tests/integration_test.go +++ b/tests/integration_test.go @@ -928,7 +928,7 @@ func TestBulkDeleteFeedbackRecords(t *testing.T) { require.NoError(t, resp.Body.Close()) } - // Bulk delete by user_id + // Bulk delete without tenant_id is rejected to avoid cross-tenant deletion. bulkDelURL := server.URL + "/v1/feedback-records?user_id=" + userID req, err := http.NewRequestWithContext(context.Background(), http.MethodDelete, bulkDelURL, http.NoBody) require.NoError(t, err) @@ -936,6 +936,17 @@ func TestBulkDeleteFeedbackRecords(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + require.NoError(t, resp.Body.Close()) + + // Bulk delete by user_id and tenant_id. + bulkDelURL = server.URL + "/v1/feedback-records?user_id=" + userID + "&tenant_id=" + tenantID + req, err = http.NewRequestWithContext(context.Background(), http.MethodDelete, bulkDelURL, http.NoBody) + require.NoError(t, err) + req.Header.Set("Authorization", "Bearer "+testAPIKey) + + resp, err = client.Do(req) + require.NoError(t, err) assert.Equal(t, http.StatusOK, resp.StatusCode) var bulkResp models.BulkDeleteResponse @@ -958,7 +969,7 @@ func TestBulkDeleteFeedbackRecords(t *testing.T) { } // Bulk delete again (no matching records) returns 0 - bulkDelURL2 := server.URL + "/v1/feedback-records?user_id=" + userID + bulkDelURL2 := server.URL + "/v1/feedback-records?user_id=" + userID + "&tenant_id=" + tenantID req2, err := http.NewRequestWithContext(context.Background(), http.MethodDelete, bulkDelURL2, http.NoBody) require.NoError(t, err) req2.Header.Set("Authorization", "Bearer "+testAPIKey) @@ -1093,7 +1104,7 @@ func TestFeedbackRecordsRepository_BulkDelete(t *testing.T) { require.NotEmpty(t, rec2.ID) // BulkDelete returns the deleted IDs - deletedIDs, err := repo.BulkDelete(ctx, userID, nil) + deletedIDs, err := repo.BulkDelete(ctx, userID, bulkDeleteTenant) require.NoError(t, err) require.Len(t, deletedIDs, 2) ids := map[string]bool{deletedIDs[0].String(): true, deletedIDs[1].String(): true} @@ -1109,8 +1120,10 @@ func TestWebhooksCRUD(t *testing.T) { client := &http.Client{} // Create webhook (no signing key = auto-generated) + webhookTenantID := "org-123" createBody := map[string]any{ "url": testWebhookURL, + "tenant_id": webhookTenantID, "event_types": []string{"feedback_record.created", "feedback_record.updated"}, } body, err := json.Marshal(createBody) @@ -1133,6 +1146,8 @@ func TestWebhooksCRUD(t *testing.T) { assert.Equal(t, testWebhookURL, created.URL) assert.NotEmpty(t, created.SigningKey) assert.True(t, created.Enabled) + require.NotNil(t, created.TenantID) + assert.Equal(t, webhookTenantID, *created.TenantID) assert.Len(t, created.EventTypes, 2) // Get webhook @@ -1202,7 +1217,7 @@ func TestWebhooksCRUD(t *testing.T) { updateBody := map[string]any{ "url": testWebhookURLV2, "enabled": false, - "tenant_id": "org-123", + "tenant_id": "org-456", } updateJSON, err := json.Marshal(updateBody) require.NoError(t, err) @@ -1224,9 +1239,9 @@ func TestWebhooksCRUD(t *testing.T) { assert.Equal(t, testWebhookURLV2, updated.URL) assert.False(t, updated.Enabled) require.NotNil(t, updated.TenantID) - assert.Equal(t, "org-123", *updated.TenantID) + assert.Equal(t, "org-456", *updated.TenantID) - // PATCH tenant_id to empty string to clear it + // PATCH tenant_id to empty string is rejected; webhooks cannot be global. clearTenantBody := map[string]any{"tenant_id": ""} clearTenantJSON, err := json.Marshal(clearTenantBody) require.NoError(t, err) @@ -1238,14 +1253,8 @@ func TestWebhooksCRUD(t *testing.T) { clearTenantReq.Header.Set("Content-Type", "application/json") clearTenantResp, err := client.Do(clearTenantReq) require.NoError(t, err) - assert.Equal(t, http.StatusOK, clearTenantResp.StatusCode) - - var afterClear models.Webhook - - err = decodeData(clearTenantResp, &afterClear) - require.NoError(t, err) + assert.Equal(t, http.StatusBadRequest, clearTenantResp.StatusCode) require.NoError(t, clearTenantResp.Body.Close()) - assert.Nil(t, afterClear.TenantID) // Delete webhook deleteWebhookURL := fmt.Sprintf("%s/v1/webhooks/%s", server.URL, created.ID) @@ -1268,6 +1277,34 @@ func TestWebhooksCRUD(t *testing.T) { require.NoError(t, getAfterResp.Body.Close()) } +func TestWebhooksCreateRequiresTenantID(t *testing.T) { + server, cleanup := setupTestServer(t) + defer cleanup() + + body, err := json.Marshal(map[string]any{ + "url": testWebhookURL, + "event_types": []string{"feedback_record.created"}, + }) + require.NoError(t, err) + + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL+"/v1/webhooks", bytes.NewBuffer(body)) + require.NoError(t, err) + req.Header.Set("Authorization", "Bearer "+testAPIKey) + req.Header.Set("Content-Type", "application/json") + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + + var problem response.ProblemDetails + + err = json.NewDecoder(resp.Body).Decode(&problem) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, "Validation Error", problem.Title) + assert.Contains(t, problem.Detail, "TenantID is required") +} + // TestWebhooksInvalidSigningKey asserts that create and update reject invalid signing_key with 400. func TestWebhooksInvalidSigningKey(t *testing.T) { server, cleanup := setupTestServer(t) @@ -1279,6 +1316,7 @@ func TestWebhooksInvalidSigningKey(t *testing.T) { createBody := map[string]any{ "url": testWebhookURL, "signing_key": "not-valid", + "tenant_id": "org-123", "event_types": []string{"feedback_record.created"}, } body, err := json.Marshal(createBody) @@ -1306,6 +1344,7 @@ func TestWebhooksInvalidSigningKey(t *testing.T) { // Create a valid webhook first for update test validBody := map[string]any{ "url": testWebhookURL, + "tenant_id": "org-123", "event_types": []string{"feedback_record.created"}, } validJSON, err := json.Marshal(validBody) diff --git a/tests/webhooks_repository_test.go b/tests/webhooks_repository_test.go index 8b3b4a1..6459891 100644 --- a/tests/webhooks_repository_test.go +++ b/tests/webhooks_repository_test.go @@ -17,10 +17,9 @@ import ( "github.com/formbricks/hub/pkg/database" ) -const repositoryWebhookScopeURLPrefix = "https://tenant-scope.test/" - func TestWebhooksRepository_ListEnabledForEventTypeAndTenant(t *testing.T) { ctx := context.Background() + urlPrefix := "https://tenant-scope.test/" + uuid.NewString() + "/" databaseURL := os.Getenv("DATABASE_URL") if databaseURL == "" { @@ -41,7 +40,7 @@ func TestWebhooksRepository_ListEnabledForEventTypeAndTenant(t *testing.T) { defer db.Close() cleanupRepositoryWebhookScopeTestRows := func() { - _, cleanupErr := db.Exec(ctx, "DELETE FROM webhooks WHERE url LIKE $1", repositoryWebhookScopeURLPrefix+"%") + _, cleanupErr := db.Exec(ctx, "DELETE FROM webhooks WHERE url LIKE $1", urlPrefix+"%") require.NoError(t, cleanupErr) } @@ -55,40 +54,36 @@ func TestWebhooksRepository_ListEnabledForEventTypeAndTenant(t *testing.T) { feedbackCreated := []datatypes.EventType{datatypes.FeedbackRecordCreated} feedbackUpdated := []datatypes.EventType{datatypes.FeedbackRecordUpdated} - globalWebhook := createWebhookForRepositoryScopeTest(ctx, t, repo, "global", nil, nil) - tenantAWebhook := createWebhookForRepositoryScopeTest(ctx, t, repo, "tenant-a", &tenantA, feedbackCreated) - tenantBWebhook := createWebhookForRepositoryScopeTest(ctx, t, repo, "tenant-b", &tenantB, feedbackCreated) - disabledTenantAWebhook := createWebhookForRepositoryScopeTest(ctx, t, repo, "disabled-a", &tenantA, feedbackCreated) + tenantAWebhook := createWebhookForRepositoryScopeTest(ctx, t, repo, urlPrefix, "tenant-a", &tenantA, feedbackCreated) + tenantBWebhook := createWebhookForRepositoryScopeTest(ctx, t, repo, urlPrefix, "tenant-b", &tenantB, feedbackCreated) + disabledTenantAWebhook := createWebhookForRepositoryScopeTest(ctx, t, repo, urlPrefix, "disabled-a", &tenantA, feedbackCreated) _, err = repo.Update(ctx, disabledTenantAWebhook.ID, &models.UpdateWebhookRequest{Enabled: &disabled}) require.NoError(t, err) - createWebhookForRepositoryScopeTest(ctx, t, repo, "updated-only-a", &tenantA, feedbackUpdated) + createWebhookForRepositoryScopeTest(ctx, t, repo, urlPrefix, "updated-only-a", &tenantA, feedbackUpdated) tenantAWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), &tenantA) require.NoError(t, err) - assertRepositoryScopeWebhookIDs(t, tenantAWebhooks, map[uuid.UUID]bool{ - globalWebhook.ID: true, + assertRepositoryScopeWebhookIDs(t, tenantAWebhooks, urlPrefix, map[uuid.UUID]bool{ tenantAWebhook.ID: true, }) tenantBWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), &tenantB) require.NoError(t, err) - assertRepositoryScopeWebhookIDs(t, tenantBWebhooks, map[uuid.UUID]bool{ - globalWebhook.ID: true, + assertRepositoryScopeWebhookIDs(t, tenantBWebhooks, urlPrefix, map[uuid.UUID]bool{ tenantBWebhook.ID: true, }) - globalOnlyWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), nil) + tenantlessWebhooks, err := repo.ListEnabledForEventTypeAndTenant(ctx, datatypes.FeedbackRecordCreated.String(), nil) require.NoError(t, err) - assertRepositoryScopeWebhookIDs(t, globalOnlyWebhooks, map[uuid.UUID]bool{ - globalWebhook.ID: true, - }) + assertRepositoryScopeWebhookIDs(t, tenantlessWebhooks, urlPrefix, map[uuid.UUID]bool{}) } func createWebhookForRepositoryScopeTest( ctx context.Context, t *testing.T, repo *repository.WebhooksRepository, + urlPrefix string, path string, tenantID *string, eventTypes []datatypes.EventType, @@ -96,7 +91,7 @@ func createWebhookForRepositoryScopeTest( t.Helper() webhook, err := repo.Create(ctx, &models.CreateWebhookRequest{ - URL: repositoryWebhookScopeURLPrefix + path, + URL: urlPrefix + path, SigningKey: "whsec_abcdefghijklmnopqrstuvwxyz123456", TenantID: tenantID, EventTypes: eventTypes, @@ -106,12 +101,12 @@ func createWebhookForRepositoryScopeTest( return webhook } -func assertRepositoryScopeWebhookIDs(t *testing.T, webhooks []models.Webhook, wantIDs map[uuid.UUID]bool) { +func assertRepositoryScopeWebhookIDs(t *testing.T, webhooks []models.Webhook, urlPrefix string, wantIDs map[uuid.UUID]bool) { t.Helper() gotIDs := make(map[uuid.UUID]bool, len(webhooks)) for _, webhook := range webhooks { - if !strings.HasPrefix(webhook.URL, repositoryWebhookScopeURLPrefix) { + if !strings.HasPrefix(webhook.URL, urlPrefix) { continue } From f19b0a1e000ecb25bb486b0817028e3125f8745a Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Tue, 28 Apr 2026 21:47:25 +0000 Subject: [PATCH 07/12] chore: revert delete api --- .../api/handlers/feedback_records_handler.go | 18 +-- .../handlers/feedback_records_handler_test.go | 48 ++----- internal/models/feedback_records.go | 9 +- .../repository/feedback_records_repository.go | 37 +++-- .../feedback_records_repository_test.go | 4 +- internal/service/feedback_records_service.go | 40 +++--- .../service/feedback_records_service_test.go | 88 +++++++++--- internal/service/webhooks_service_test.go | 9 ++ openapi.yaml | 12 +- tests/integration_test.go | 135 ++++++------------ 10 files changed, 196 insertions(+), 204 deletions(-) diff --git a/internal/api/handlers/feedback_records_handler.go b/internal/api/handlers/feedback_records_handler.go index c085157..d8929a5 100644 --- a/internal/api/handlers/feedback_records_handler.go +++ b/internal/api/handlers/feedback_records_handler.go @@ -25,7 +25,7 @@ type FeedbackRecordsService interface { ListFeedbackRecords(ctx context.Context, filters *models.ListFeedbackRecordsFilters) (*models.ListFeedbackRecordsResponse, error) UpdateFeedbackRecord(ctx context.Context, id uuid.UUID, req *models.UpdateFeedbackRecordRequest) (*models.FeedbackRecord, error) DeleteFeedbackRecord(ctx context.Context, id uuid.UUID) error - BulkDeleteFeedbackRecords(ctx context.Context, userID string, tenantID *string) (int, error) + BulkDeleteFeedbackRecords(ctx context.Context, userID string) (int, error) } // FeedbackRecordsHandler handles HTTP requests for feedback records. @@ -220,7 +220,7 @@ func (h *FeedbackRecordsHandler) Delete(w http.ResponseWriter, r *http.Request) w.WriteHeader(http.StatusNoContent) } -// BulkDelete handles DELETE /v1/feedback-records?user_id=&tenant_id=. +// BulkDelete handles DELETE /v1/feedback-records?user_id=. func (h *FeedbackRecordsHandler) BulkDelete(w http.ResponseWriter, r *http.Request) { filters := &models.BulkDeleteFilters{} @@ -231,24 +231,12 @@ func (h *FeedbackRecordsHandler) BulkDelete(w http.ResponseWriter, r *http.Reque return } - deletedCount, err := h.service.BulkDeleteFeedbackRecords(r.Context(), filters.UserID, filters.TenantID) + deletedCount, err := h.service.BulkDeleteFeedbackRecords(r.Context(), filters.UserID) if err != nil { - if errors.Is(err, huberrors.ErrValidation) { - validation.RespondValidationError(w, err) - - return - } - - var tenantID string - if filters.TenantID != nil { - tenantID = *filters.TenantID - } - slog.Error("Failed to bulk delete feedback records", // #nosec G706 -- slog key-values "method", r.Method, "path", r.URL.Path, "user_id", filters.UserID, - "tenant_id", tenantID, "error", err, ) diff --git a/internal/api/handlers/feedback_records_handler_test.go b/internal/api/handlers/feedback_records_handler_test.go index 24632f7..9367c72 100644 --- a/internal/api/handlers/feedback_records_handler_test.go +++ b/internal/api/handlers/feedback_records_handler_test.go @@ -16,7 +16,7 @@ import ( // mockFeedbackRecordsService mocks FeedbackRecordsService for handler tests. type mockFeedbackRecordsService struct { - bulkDeleteFunc func(ctx context.Context, userID string, tenantID *string) (int, error) + bulkDeleteFunc func(ctx context.Context, userID string) (int, error) } func (m *mockFeedbackRecordsService) CreateFeedbackRecord( @@ -45,9 +45,9 @@ func (m *mockFeedbackRecordsService) DeleteFeedbackRecord(context.Context, uuid. return nil } -func (m *mockFeedbackRecordsService) BulkDeleteFeedbackRecords(ctx context.Context, userID string, tenantID *string) (int, error) { +func (m *mockFeedbackRecordsService) BulkDeleteFeedbackRecords(ctx context.Context, userID string) (int, error) { if m.bulkDeleteFunc != nil { - return m.bulkDeleteFunc(ctx, userID, tenantID) + return m.bulkDeleteFunc(ctx, userID) } return 0, nil @@ -70,10 +70,8 @@ func TestFeedbackRecordsHandler_List(t *testing.T) { func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { t.Run("success returns 200 with deleted_count and message", func(t *testing.T) { mock := &mockFeedbackRecordsService{ - bulkDeleteFunc: func(_ context.Context, userID string, tenantID *string) (int, error) { + bulkDeleteFunc: func(_ context.Context, userID string) (int, error) { assert.Equal(t, "user-123", userID) - require.NotNil(t, tenantID) - assert.Equal(t, "tenant-a", *tenantID) return 3, nil }, @@ -81,7 +79,7 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { handler := NewFeedbackRecordsHandler(mock) req := httptest.NewRequestWithContext(context.Background(), - http.MethodDelete, "http://test/v1/feedback-records?user_id=user-123&tenant_id=tenant-a", http.NoBody) + http.MethodDelete, "http://test/v1/feedback-records?user_id=user-123", http.NoBody) rec := httptest.NewRecorder() handler.BulkDelete(rec, req) @@ -96,15 +94,11 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { assert.Equal(t, "Successfully deleted 3 feedback records", resp.Message) }) - t.Run("success with tenant_id passes tenant to service", func(t *testing.T) { - var capturedTenantID *string - + t.Run("extra tenant_id query parameter is ignored", func(t *testing.T) { mock := &mockFeedbackRecordsService{ - bulkDeleteFunc: func(_ context.Context, userID string, tenantID *string) (int, error) { + bulkDeleteFunc: func(_ context.Context, userID string) (int, error) { assert.Equal(t, "user-456", userID) - capturedTenantID = tenantID - return 1, nil }, } @@ -117,29 +111,13 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { handler.BulkDelete(rec, req) assert.Equal(t, http.StatusOK, rec.Code) - require.NotNil(t, capturedTenantID) - assert.Equal(t, "tenant-a", *capturedTenantID) - }) - - t.Run("missing tenant_id returns bad request", func(t *testing.T) { - mock := &mockFeedbackRecordsService{} - handler := NewFeedbackRecordsHandler(mock) - - req := httptest.NewRequestWithContext(context.Background(), - http.MethodDelete, "http://test/v1/feedback-records?user_id=user-123", http.NoBody) - rec := httptest.NewRecorder() - - handler.BulkDelete(rec, req) - - assert.Equal(t, http.StatusBadRequest, rec.Code) }) t.Run("missing user_id returns bad request", func(t *testing.T) { mock := &mockFeedbackRecordsService{} handler := NewFeedbackRecordsHandler(mock) - req := httptest.NewRequestWithContext(context.Background(), - http.MethodDelete, "http://test/v1/feedback-records?tenant_id=tenant-a", http.NoBody) + req := httptest.NewRequestWithContext(context.Background(), http.MethodDelete, "http://test/v1/feedback-records", http.NoBody) rec := httptest.NewRecorder() handler.BulkDelete(rec, req) @@ -152,7 +130,7 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { handler := NewFeedbackRecordsHandler(mock) req := httptest.NewRequestWithContext(context.Background(), - http.MethodDelete, "http://test/v1/feedback-records?user_id=&tenant_id=tenant-a", http.NoBody) + http.MethodDelete, "http://test/v1/feedback-records?user_id=", http.NoBody) rec := httptest.NewRecorder() handler.BulkDelete(rec, req) @@ -162,14 +140,14 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { t.Run("service error returns 500", func(t *testing.T) { mock := &mockFeedbackRecordsService{ - bulkDeleteFunc: func(_ context.Context, _ string, _ *string) (int, error) { + bulkDeleteFunc: func(_ context.Context, _ string) (int, error) { return 0, assert.AnError }, } handler := NewFeedbackRecordsHandler(mock) req := httptest.NewRequestWithContext(context.Background(), - http.MethodDelete, "http://test/v1/feedback-records?user_id=user-789&tenant_id=tenant-a", http.NoBody) + http.MethodDelete, "http://test/v1/feedback-records?user_id=user-789", http.NoBody) rec := httptest.NewRecorder() handler.BulkDelete(rec, req) @@ -179,14 +157,14 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { t.Run("zero deleted returns 200 with deleted_count 0", func(t *testing.T) { mock := &mockFeedbackRecordsService{ - bulkDeleteFunc: func(_ context.Context, _ string, _ *string) (int, error) { + bulkDeleteFunc: func(_ context.Context, _ string) (int, error) { return 0, nil }, } handler := NewFeedbackRecordsHandler(mock) req := httptest.NewRequestWithContext(context.Background(), - http.MethodDelete, "http://test/v1/feedback-records?user_id=nonexistent&tenant_id=tenant-a", http.NoBody) + http.MethodDelete, "http://test/v1/feedback-records?user_id=nonexistent", http.NoBody) rec := httptest.NewRecorder() handler.BulkDelete(rec, req) diff --git a/internal/models/feedback_records.go b/internal/models/feedback_records.go index c2805b6..73aff65 100644 --- a/internal/models/feedback_records.go +++ b/internal/models/feedback_records.go @@ -198,8 +198,7 @@ type ListFeedbackRecordsResponse struct { // BulkDeleteFilters represents query parameters for bulk delete operation. type BulkDeleteFilters struct { - UserID string `form:"user_id" validate:"required,no_null_bytes,min=1"` - TenantID *string `form:"tenant_id" validate:"required,no_null_bytes,min=1"` + UserID string `form:"user_id" validate:"required,no_null_bytes,min=1"` } // BulkDeleteResponse represents the response for bulk delete operation. @@ -207,3 +206,9 @@ type BulkDeleteResponse struct { DeletedCount int64 `json:"deleted_count"` Message string `json:"message"` } + +// DeletedFeedbackRecordsByTenant groups deleted feedback record IDs by tenant. +type DeletedFeedbackRecordsByTenant struct { + TenantID string + IDs []uuid.UUID +} diff --git a/internal/repository/feedback_records_repository.go b/internal/repository/feedback_records_repository.go index 247abb4..965b766 100644 --- a/internal/repository/feedback_records_repository.go +++ b/internal/repository/feedback_records_repository.go @@ -394,37 +394,50 @@ func (r *FeedbackRecordsRepository) Delete(ctx context.Context, id uuid.UUID) er return nil } -// BulkDelete deletes all feedback records matching user_id and tenant_id. -// It returns the deleted IDs (via RETURNING id) so callers can e.g. publish events. -func (r *FeedbackRecordsRepository) BulkDelete(ctx context.Context, userID, tenantID string) ([]uuid.UUID, error) { +// BulkDelete deletes all feedback records matching user_id. +// It returns deleted IDs grouped by tenant so callers can publish tenant-scoped side effects. +func (r *FeedbackRecordsRepository) BulkDelete( + ctx context.Context, userID string, +) ([]models.DeletedFeedbackRecordsByTenant, error) { query := ` DELETE FROM feedback_records - WHERE user_id = $1 AND tenant_id = $2 - RETURNING id` - args := []any{userID, tenantID} + WHERE user_id = $1 + RETURNING id, tenant_id` - rows, err := r.db.Query(ctx, query, args...) + rows, err := r.db.Query(ctx, query, userID) if err != nil { return nil, fmt.Errorf("failed to bulk delete feedback records: %w", err) } defer rows.Close() - var ids []uuid.UUID + groups := make([]models.DeletedFeedbackRecordsByTenant, 0) + groupIndexByTenant := make(map[string]int) for rows.Next() { - var id uuid.UUID - if err := rows.Scan(&id); err != nil { + var ( + id uuid.UUID + tenantID string + ) + + if err := rows.Scan(&id, &tenantID); err != nil { return nil, fmt.Errorf("failed to scan deleted feedback record id: %w", err) } - ids = append(ids, id) + groupIndex, ok := groupIndexByTenant[tenantID] + if !ok { + groupIndex = len(groups) + groupIndexByTenant[tenantID] = groupIndex + groups = append(groups, models.DeletedFeedbackRecordsByTenant{TenantID: tenantID}) + } + + groups[groupIndex].IDs = append(groups[groupIndex].IDs, id) } if err := rows.Err(); err != nil { return nil, fmt.Errorf("error iterating bulk delete result: %w", err) } - return ids, nil + return groups, nil } // fetchFeedbackRecords executes the given query and scans rows into FeedbackRecord slices. diff --git a/internal/repository/feedback_records_repository_test.go b/internal/repository/feedback_records_repository_test.go index bec5f99..a0a2653 100644 --- a/internal/repository/feedback_records_repository_test.go +++ b/internal/repository/feedback_records_repository_test.go @@ -6,9 +6,9 @@ import ( // BulkDelete is tested by integration tests in tests/integration_test.go: // - TestFeedbackRecordsRepository_BulkDelete exercises the repository directly and asserts -// the returned slice of deleted records. +// deleted records are returned grouped by tenant. // - TestBulkDeleteFeedbackRecords exercises the full stack (handler, service, repo) including -// tenant_id filter and response shape. +// GDPR user_id erasure across tenants and response shape. func TestFeedbackRecordsRepository_Package(_ *testing.T) { // No DB in unit tests; BulkDelete coverage is in tests/. } diff --git a/internal/service/feedback_records_service.go b/internal/service/feedback_records_service.go index 2fd3ffa..a3839d5 100644 --- a/internal/service/feedback_records_service.go +++ b/internal/service/feedback_records_service.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "log/slog" "strings" "time" @@ -35,7 +36,7 @@ type FeedbackRecordsRepository interface { ) ([]models.FeedbackRecord, bool, error) Update(ctx context.Context, id uuid.UUID, req *models.UpdateFeedbackRecordRequest) (*models.FeedbackRecord, error) Delete(ctx context.Context, id uuid.UUID) error - BulkDelete(ctx context.Context, userID, tenantID string) ([]uuid.UUID, error) + BulkDelete(ctx context.Context, userID string) ([]models.DeletedFeedbackRecordsByTenant, error) } // EmbeddingsRepository defines the interface for embeddings table access. @@ -203,31 +204,38 @@ func (s *FeedbackRecordsService) DeleteFeedbackRecord(ctx context.Context, id uu return nil } -// BulkDeleteFeedbackRecords deletes all feedback records matching user_id and tenant_id. -// Publishes a single tenant-aware FeedbackRecordDeleted event. -func (s *FeedbackRecordsService) BulkDeleteFeedbackRecords(ctx context.Context, userID string, tenantID *string) (int, error) { +// BulkDeleteFeedbackRecords deletes all feedback records matching user_id. +// It publishes one tenant-aware FeedbackRecordDeleted event per tenant represented in the deleted rows. +func (s *FeedbackRecordsService) BulkDeleteFeedbackRecords(ctx context.Context, userID string) (int, error) { if userID == "" { return 0, ErrUserIDRequired } - normalizedTenantID, err := normalizeRequiredTenantID(tenantID) - if err != nil { - return 0, err - } - - ids, err := s.repo.BulkDelete(ctx, userID, normalizedTenantID) + groups, err := s.repo.BulkDelete(ctx, userID) if err != nil { return 0, fmt.Errorf("bulk delete feedback records: %w", err) } - if len(ids) > 0 && s.publisher != nil { - s.publisher.PublishEvent(ctx, datatypes.FeedbackRecordDeleted, models.DeletedIDsEventData{ - TenantID: normalizedTenantID, - IDs: ids, - }) + deletedCount := 0 + for _, group := range groups { + deletedCount += len(group.IDs) + + if len(group.IDs) == 0 || s.publisher == nil { + continue + } + + if group.TenantID == "" { + slog.Error("bulk delete feedback records: deleted rows missing tenant_id; skipping webhook event", + "deleted_count", len(group.IDs), + ) + + continue + } + + s.publisher.PublishEvent(ctx, datatypes.FeedbackRecordDeleted, models.DeletedIDsEventData(group)) } - return len(ids), nil + return deletedCount, nil } // SetEmbedding sets or clears the embedding for a feedback record and model (internal use by embeddings worker). diff --git a/internal/service/feedback_records_service_test.go b/internal/service/feedback_records_service_test.go index 8f8b2e9..e5cb589 100644 --- a/internal/service/feedback_records_service_test.go +++ b/internal/service/feedback_records_service_test.go @@ -9,14 +9,13 @@ import ( "github.com/google/uuid" "github.com/formbricks/hub/internal/datatypes" - "github.com/formbricks/hub/internal/huberrors" "github.com/formbricks/hub/internal/models" ) type mockFeedbackRecordsRepo struct { - record *models.FeedbackRecord - bulkIDs []uuid.UUID - deletedID uuid.UUID + record *models.FeedbackRecord + bulkGroups []models.DeletedFeedbackRecordsByTenant + deletedID uuid.UUID } func (m *mockFeedbackRecordsRepo) Create( @@ -53,8 +52,8 @@ func (m *mockFeedbackRecordsRepo) Delete(_ context.Context, id uuid.UUID) error return nil } -func (m *mockFeedbackRecordsRepo) BulkDelete(_ context.Context, _, _ string) ([]uuid.UUID, error) { - return m.bulkIDs, nil +func (m *mockFeedbackRecordsRepo) BulkDelete(_ context.Context, _ string) ([]models.DeletedFeedbackRecordsByTenant, error) { + return m.bulkGroups, nil } func TestFeedbackRecordsService_DeleteFeedbackRecord_PublishesTenantAwareDeletedEvent(t *testing.T) { @@ -77,35 +76,47 @@ func TestFeedbackRecordsService_DeleteFeedbackRecord_PublishesTenantAwareDeleted assertDeletedEventData(t, publisher, datatypes.FeedbackRecordDeleted, tenantID, []uuid.UUID{recordID}) } -func TestFeedbackRecordsService_BulkDeleteFeedbackRecords_PublishesTenantAwareDeletedEvent(t *testing.T) { +func TestFeedbackRecordsService_BulkDeleteFeedbackRecords_PublishesTenantAwareDeletedEventsByTenant(t *testing.T) { ctx := context.Background() - tenantID := "org-123" - ids := []uuid.UUID{uuid.Must(uuid.NewV7()), uuid.Must(uuid.NewV7())} - repo := &mockFeedbackRecordsRepo{bulkIDs: ids} + tenantA := "org-123" + tenantB := "org-456" + tenantAIDs := []uuid.UUID{uuid.Must(uuid.NewV7()), uuid.Must(uuid.NewV7())} + tenantBIDs := []uuid.UUID{uuid.Must(uuid.NewV7())} + repo := &mockFeedbackRecordsRepo{ + bulkGroups: []models.DeletedFeedbackRecordsByTenant{ + {TenantID: tenantA, IDs: tenantAIDs}, + {TenantID: tenantB, IDs: tenantBIDs}, + }, + } publisher := &capturePublisher{} svc := NewFeedbackRecordsService(repo, nil, "", publisher, nil, "", 0) - count, err := svc.BulkDeleteFeedbackRecords(ctx, "user-123", &tenantID) + count, err := svc.BulkDeleteFeedbackRecords(ctx, "user-123") if err != nil { t.Fatalf("BulkDeleteFeedbackRecords() error = %v", err) } - if count != len(ids) { - t.Fatalf("count = %d, want %d", count, len(ids)) + if count != len(tenantAIDs)+len(tenantBIDs) { + t.Fatalf("count = %d, want %d", count, len(tenantAIDs)+len(tenantBIDs)) } - assertDeletedEventData(t, publisher, datatypes.FeedbackRecordDeleted, tenantID, ids) + assertDeletedEventDataAt(t, publisher, 0, datatypes.FeedbackRecordDeleted, tenantA, tenantAIDs) + assertDeletedEventDataAt(t, publisher, 1, datatypes.FeedbackRecordDeleted, tenantB, tenantBIDs) } -func TestFeedbackRecordsService_BulkDeleteFeedbackRecords_RequiresTenantID(t *testing.T) { +func TestFeedbackRecordsService_BulkDeleteFeedbackRecords_RequiresUserID(t *testing.T) { ctx := context.Background() - repo := &mockFeedbackRecordsRepo{bulkIDs: []uuid.UUID{uuid.Must(uuid.NewV7())}} + repo := &mockFeedbackRecordsRepo{ + bulkGroups: []models.DeletedFeedbackRecordsByTenant{ + {TenantID: "org-123", IDs: []uuid.UUID{uuid.Must(uuid.NewV7())}}, + }, + } publisher := &capturePublisher{} svc := NewFeedbackRecordsService(repo, nil, "", publisher, nil, "", 0) - count, err := svc.BulkDeleteFeedbackRecords(ctx, "user-123", nil) - if !errors.Is(err, huberrors.ErrValidation) { - t.Fatalf("BulkDeleteFeedbackRecords() error = %v, want validation error", err) + count, err := svc.BulkDeleteFeedbackRecords(ctx, "") + if !errors.Is(err, ErrUserIDRequired) { + t.Fatalf("BulkDeleteFeedbackRecords() error = %v, want ErrUserIDRequired", err) } if count != 0 { @@ -117,6 +128,45 @@ func TestFeedbackRecordsService_BulkDeleteFeedbackRecords_RequiresTenantID(t *te } } +func assertDeletedEventDataAt( + t *testing.T, + publisher *capturePublisher, + index int, + eventType datatypes.EventType, + tenantID string, + ids []uuid.UUID, +) { + t.Helper() + + if publisher.callCount <= index { + t.Fatalf("published %d events, want event at index %d", publisher.callCount, index) + } + + event := publisher.events[index] + if event.eventType != eventType { + t.Fatalf("published event type = %s, want %s", event.eventType, eventType) + } + + data, ok := event.data.(models.DeletedIDsEventData) + if !ok { + t.Fatalf("published data type = %T, want DeletedIDsEventData", event.data) + } + + if data.TenantID != tenantID { + t.Errorf("TenantID = %q, want %q", data.TenantID, tenantID) + } + + if len(data.IDs) != len(ids) { + t.Fatalf("IDs length = %d, want %d", len(data.IDs), len(ids)) + } + + for i := range ids { + if data.IDs[i] != ids[i] { + t.Errorf("IDs[%d] = %v, want %v", i, data.IDs[i], ids[i]) + } + } +} + func assertDeletedEventData( t *testing.T, publisher *capturePublisher, eventType datatypes.EventType, tenantID string, ids []uuid.UUID, ) { diff --git a/internal/service/webhooks_service_test.go b/internal/service/webhooks_service_test.go index 8269424..95305a3 100644 --- a/internal/service/webhooks_service_test.go +++ b/internal/service/webhooks_service_test.go @@ -68,12 +68,20 @@ type capturePublisher struct { data any changedFields []string callCount int + events []capturedEvent +} + +type capturedEvent struct { + eventType datatypes.EventType + data any + changedFields []string } func (p *capturePublisher) PublishEvent(_ context.Context, eventType datatypes.EventType, data any) { p.eventType = eventType p.data = data p.callCount++ + p.events = append(p.events, capturedEvent{eventType: eventType, data: data}) } func (p *capturePublisher) PublishEventWithChangedFields( @@ -83,6 +91,7 @@ func (p *capturePublisher) PublishEventWithChangedFields( p.data = data p.changedFields = changedFields p.callCount++ + p.events = append(p.events, capturedEvent{eventType: eventType, data: data, changedFields: changedFields}) } func TestWebhooksService_CreateWebhook_InvalidSigningKey(t *testing.T) { diff --git a/openapi.yaml b/openapi.yaml index 653bbc7..5a92ec6 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -381,7 +381,7 @@ paths: tags: - Feedback Records summary: Bulk delete feedback records by user ID - description: Permanently deletes all feedback record data points matching the specified user_id within the required tenant_id. This endpoint supports GDPR Article 17 (Right to Erasure) requests. + description: Permanently deletes all feedback record data points matching the specified user_id across tenants. This endpoint supports GDPR Article 17 (Right to Erasure) requests. operationId: bulk-delete-feedback-records parameters: - name: user_id @@ -394,16 +394,6 @@ paths: minLength: 1 pattern: '^[^\x00]*$' example: "user-abc-123" - - name: tenant_id - in: query - description: Tenant ID to delete within (required for isolation). NULL bytes not allowed. - required: true - schema: - type: string - description: Tenant ID to delete within (required for isolation). NULL bytes not allowed. - minLength: 1 - pattern: '^[^\x00]*$' - example: "org-123" responses: "200": description: OK diff --git a/tests/integration_test.go b/tests/integration_test.go index af78c5f..ed7f850 100644 --- a/tests/integration_test.go +++ b/tests/integration_test.go @@ -888,9 +888,10 @@ func TestBulkDeleteFeedbackRecords(t *testing.T) { userID := "bulk-delete-test-user-123" subID := uuid.New().String() // unique per run to avoid 409 from leftover data - // Create several feedback records with the same user_id - tenantID := "test-tenant" - createPayload := func(fieldID string, valueNum float64) map[string]any { + // Create several feedback records with the same user_id across tenants. + tenantA := "test-tenant-a" + tenantB := "test-tenant-b" + createPayload := func(fieldID, tenantID string, valueNum float64) map[string]any { return map[string]any{ "source_type": "formbricks", "submission_id": subID, @@ -904,9 +905,9 @@ func TestBulkDeleteFeedbackRecords(t *testing.T) { createdIDs := make([]string, 0, 3) for i, p := range []map[string]any{ - createPayload("nps_1", 8), - createPayload("nps_2", 9), - createPayload("nps_3", 10), + createPayload("nps_1", tenantA, 8), + createPayload("nps_2", tenantB, 9), + createPayload("nps_3", tenantB, 10), } { body, err := json.Marshal(p) require.NoError(t, err) @@ -928,7 +929,7 @@ func TestBulkDeleteFeedbackRecords(t *testing.T) { require.NoError(t, resp.Body.Close()) } - // Bulk delete without tenant_id is rejected to avoid cross-tenant deletion. + // Bulk delete by user_id deletes every matching record for GDPR erasure, regardless of tenant. bulkDelURL := server.URL + "/v1/feedback-records?user_id=" + userID req, err := http.NewRequestWithContext(context.Background(), http.MethodDelete, bulkDelURL, http.NoBody) require.NoError(t, err) @@ -936,17 +937,6 @@ func TestBulkDeleteFeedbackRecords(t *testing.T) { resp, err := client.Do(req) require.NoError(t, err) - assert.Equal(t, http.StatusBadRequest, resp.StatusCode) - require.NoError(t, resp.Body.Close()) - - // Bulk delete by user_id and tenant_id. - bulkDelURL = server.URL + "/v1/feedback-records?user_id=" + userID + "&tenant_id=" + tenantID - req, err = http.NewRequestWithContext(context.Background(), http.MethodDelete, bulkDelURL, http.NoBody) - require.NoError(t, err) - req.Header.Set("Authorization", "Bearer "+testAPIKey) - - resp, err = client.Do(req) - require.NoError(t, err) assert.Equal(t, http.StatusOK, resp.StatusCode) var bulkResp models.BulkDeleteResponse @@ -969,7 +959,7 @@ func TestBulkDeleteFeedbackRecords(t *testing.T) { } // Bulk delete again (no matching records) returns 0 - bulkDelURL2 := server.URL + "/v1/feedback-records?user_id=" + userID + "&tenant_id=" + tenantID + bulkDelURL2 := server.URL + "/v1/feedback-records?user_id=" + userID req2, err := http.NewRequestWithContext(context.Background(), http.MethodDelete, bulkDelURL2, http.NoBody) require.NoError(t, err) req2.Header.Set("Authorization", "Bearer "+testAPIKey) @@ -983,70 +973,6 @@ func TestBulkDeleteFeedbackRecords(t *testing.T) { require.NoError(t, err) require.NoError(t, resp2.Body.Close()) assert.Equal(t, int64(0), bulkResp2.DeletedCount) - - // Bulk delete with tenant_id: only records for that tenant are deleted - t.Run("Bulk delete with tenant_id filter", func(t *testing.T) { - tenantA, tenantB := "tenant-bulk-a", "tenant-bulk-b" - userIDTenant := "bulk-delete-tenant-user" - - // Create one record with tenant_a, two with tenant_b - for _, item := range []struct { - tenantID string - fieldID string - }{ - {tenantA, "fa"}, - {tenantB, "fb1"}, - {tenantB, "fb2"}, - } { - body, err := json.Marshal(map[string]any{ - "source_type": "formbricks", - "submission_id": userIDTenant + "-" + item.fieldID, - "user_id": userIDTenant, - "tenant_id": item.tenantID, - "field_id": item.fieldID, - "field_type": "text", - "value_text": "x", - }) - require.NoError(t, err) - req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL+"/v1/feedback-records", bytes.NewBuffer(body)) - require.NoError(t, err) - req.Header.Set("Authorization", "Bearer "+testAPIKey) - req.Header.Set("Content-Type", "application/json") - resp, err := client.Do(req) - require.NoError(t, err) - require.Equal(t, http.StatusCreated, resp.StatusCode) - require.NoError(t, resp.Body.Close()) - } - - // Delete only tenant_a - delURL := server.URL + "/v1/feedback-records?user_id=" + userIDTenant + "&tenant_id=" + tenantA - delReq, err := http.NewRequestWithContext(context.Background(), http.MethodDelete, delURL, http.NoBody) - require.NoError(t, err) - delReq.Header.Set("Authorization", "Bearer "+testAPIKey) - delResp, err := client.Do(delReq) - require.NoError(t, err) - require.Equal(t, http.StatusOK, delResp.StatusCode) - - var delResult models.BulkDeleteResponse - - err = decodeData(delResp, &delResult) - require.NoError(t, err) - require.NoError(t, delResp.Body.Close()) - assert.Equal(t, int64(1), delResult.DeletedCount) - - // Delete remaining (tenant_b) — should delete 2 - delURL2 := server.URL + "/v1/feedback-records?user_id=" + userIDTenant + "&tenant_id=" + tenantB - delReq2, err := http.NewRequestWithContext(context.Background(), http.MethodDelete, delURL2, http.NoBody) - require.NoError(t, err) - delReq2.Header.Set("Authorization", "Bearer "+testAPIKey) - delResp2, err := client.Do(delReq2) - require.NoError(t, err) - require.Equal(t, http.StatusOK, delResp2.StatusCode) - err = decodeData(delResp2, &delResult) - require.NoError(t, err) - require.NoError(t, delResp2.Body.Close()) - assert.Equal(t, int64(2), delResult.DeletedCount) - }) } // TestFeedbackRecordsRepository_BulkDelete tests the repository BulkDelete return value (deleted IDs). @@ -1074,9 +1000,11 @@ func TestFeedbackRecordsRepository_BulkDelete(t *testing.T) { userID := "repo-bulk-delete-user" sourceType := "formbricks" - // Create two records with same user_id + // Create records with same user_id across tenants. const bulkDeleteTenant = "bulk-delete-tenant" + const otherBulkDeleteTenant = "bulk-delete-tenant-other" + req1 := &models.CreateFeedbackRecordRequest{ SourceType: sourceType, SubmissionID: userID, @@ -1084,7 +1012,7 @@ func TestFeedbackRecordsRepository_BulkDelete(t *testing.T) { FieldID: "f1", FieldType: models.FieldTypeNumber, ValueNumber: new(1.0), - UserID: new(userID), + UserID: &userID, } rec1, err := repo.Create(ctx, req1) require.NoError(t, err) @@ -1097,19 +1025,42 @@ func TestFeedbackRecordsRepository_BulkDelete(t *testing.T) { FieldID: "f2", FieldType: models.FieldTypeNumber, ValueNumber: new(2.0), - UserID: new(userID), + UserID: &userID, } rec2, err := repo.Create(ctx, req2) require.NoError(t, err) require.NotEmpty(t, rec2.ID) - // BulkDelete returns the deleted IDs - deletedIDs, err := repo.BulkDelete(ctx, userID, bulkDeleteTenant) + valueText := "delete me too" + req3 := &models.CreateFeedbackRecordRequest{ + SourceType: sourceType, + SubmissionID: userID, + TenantID: otherBulkDeleteTenant, + FieldID: "f3", + FieldType: models.FieldTypeText, + ValueText: &valueText, + UserID: &userID, + } + rec3, err := repo.Create(ctx, req3) require.NoError(t, err) - require.Len(t, deletedIDs, 2) - ids := map[string]bool{deletedIDs[0].String(): true, deletedIDs[1].String(): true} - assert.True(t, ids[rec1.ID.String()]) - assert.True(t, ids[rec2.ID.String()]) + require.NotEmpty(t, rec3.ID) + + // BulkDelete returns deleted IDs grouped by tenant for tenant-safe side effects. + deletedGroups, err := repo.BulkDelete(ctx, userID) + require.NoError(t, err) + require.Len(t, deletedGroups, 2) + + idsByTenant := map[string]map[string]bool{} + for _, group := range deletedGroups { + idsByTenant[group.TenantID] = map[string]bool{} + for _, id := range group.IDs { + idsByTenant[group.TenantID][id.String()] = true + } + } + + assert.True(t, idsByTenant[bulkDeleteTenant][rec1.ID.String()]) + assert.True(t, idsByTenant[bulkDeleteTenant][rec2.ID.String()]) + assert.True(t, idsByTenant[otherBulkDeleteTenant][rec3.ID.String()]) } // TestWebhooksCRUD tests webhook create, get, list, update, delete. From 4cd752bc383c57ca6a7bbbfb53c0cc0eb7746cbe Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Tue, 28 Apr 2026 22:00:13 +0000 Subject: [PATCH 08/12] chore: allow deleting specific tenant id --- .../api/handlers/feedback_records_handler.go | 18 ++- .../handlers/feedback_records_handler_test.go | 38 ++++-- internal/models/feedback_records.go | 3 +- .../repository/feedback_records_repository.go | 16 ++- .../feedback_records_repository_test.go | 4 +- internal/service/feedback_records_service.go | 21 ++- .../service/feedback_records_service_test.go | 64 ++++++++- openapi.yaml | 15 ++- tests/integration_test.go | 126 ++++++++++++------ 9 files changed, 231 insertions(+), 74 deletions(-) diff --git a/internal/api/handlers/feedback_records_handler.go b/internal/api/handlers/feedback_records_handler.go index d8929a5..90edc20 100644 --- a/internal/api/handlers/feedback_records_handler.go +++ b/internal/api/handlers/feedback_records_handler.go @@ -25,7 +25,7 @@ type FeedbackRecordsService interface { ListFeedbackRecords(ctx context.Context, filters *models.ListFeedbackRecordsFilters) (*models.ListFeedbackRecordsResponse, error) UpdateFeedbackRecord(ctx context.Context, id uuid.UUID, req *models.UpdateFeedbackRecordRequest) (*models.FeedbackRecord, error) DeleteFeedbackRecord(ctx context.Context, id uuid.UUID) error - BulkDeleteFeedbackRecords(ctx context.Context, userID string) (int, error) + BulkDeleteFeedbackRecords(ctx context.Context, filters *models.BulkDeleteFilters) (int, error) } // FeedbackRecordsHandler handles HTTP requests for feedback records. @@ -220,7 +220,7 @@ func (h *FeedbackRecordsHandler) Delete(w http.ResponseWriter, r *http.Request) w.WriteHeader(http.StatusNoContent) } -// BulkDelete handles DELETE /v1/feedback-records?user_id=. +// BulkDelete handles DELETE /v1/feedback-records?user_id=[&tenant_id=]. func (h *FeedbackRecordsHandler) BulkDelete(w http.ResponseWriter, r *http.Request) { filters := &models.BulkDeleteFilters{} @@ -231,12 +231,24 @@ func (h *FeedbackRecordsHandler) BulkDelete(w http.ResponseWriter, r *http.Reque return } - deletedCount, err := h.service.BulkDeleteFeedbackRecords(r.Context(), filters.UserID) + deletedCount, err := h.service.BulkDeleteFeedbackRecords(r.Context(), filters) if err != nil { + if errors.Is(err, huberrors.ErrValidation) { + validation.RespondValidationError(w, err) + + return + } + + var tenantID string + if filters.TenantID != nil { + tenantID = *filters.TenantID + } + slog.Error("Failed to bulk delete feedback records", // #nosec G706 -- slog key-values "method", r.Method, "path", r.URL.Path, "user_id", filters.UserID, + "tenant_id", tenantID, "error", err, ) diff --git a/internal/api/handlers/feedback_records_handler_test.go b/internal/api/handlers/feedback_records_handler_test.go index 9367c72..a2e8758 100644 --- a/internal/api/handlers/feedback_records_handler_test.go +++ b/internal/api/handlers/feedback_records_handler_test.go @@ -16,7 +16,7 @@ import ( // mockFeedbackRecordsService mocks FeedbackRecordsService for handler tests. type mockFeedbackRecordsService struct { - bulkDeleteFunc func(ctx context.Context, userID string) (int, error) + bulkDeleteFunc func(ctx context.Context, filters *models.BulkDeleteFilters) (int, error) } func (m *mockFeedbackRecordsService) CreateFeedbackRecord( @@ -45,9 +45,11 @@ func (m *mockFeedbackRecordsService) DeleteFeedbackRecord(context.Context, uuid. return nil } -func (m *mockFeedbackRecordsService) BulkDeleteFeedbackRecords(ctx context.Context, userID string) (int, error) { +func (m *mockFeedbackRecordsService) BulkDeleteFeedbackRecords( + ctx context.Context, filters *models.BulkDeleteFilters, +) (int, error) { if m.bulkDeleteFunc != nil { - return m.bulkDeleteFunc(ctx, userID) + return m.bulkDeleteFunc(ctx, filters) } return 0, nil @@ -70,8 +72,9 @@ func TestFeedbackRecordsHandler_List(t *testing.T) { func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { t.Run("success returns 200 with deleted_count and message", func(t *testing.T) { mock := &mockFeedbackRecordsService{ - bulkDeleteFunc: func(_ context.Context, userID string) (int, error) { - assert.Equal(t, "user-123", userID) + bulkDeleteFunc: func(_ context.Context, filters *models.BulkDeleteFilters) (int, error) { + assert.Equal(t, "user-123", filters.UserID) + assert.Nil(t, filters.TenantID) return 3, nil }, @@ -94,10 +97,12 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { assert.Equal(t, "Successfully deleted 3 feedback records", resp.Message) }) - t.Run("extra tenant_id query parameter is ignored", func(t *testing.T) { + t.Run("optional tenant_id query parameter is passed to service", func(t *testing.T) { mock := &mockFeedbackRecordsService{ - bulkDeleteFunc: func(_ context.Context, userID string) (int, error) { - assert.Equal(t, "user-456", userID) + bulkDeleteFunc: func(_ context.Context, filters *models.BulkDeleteFilters) (int, error) { + assert.Equal(t, "user-456", filters.UserID) + require.NotNil(t, filters.TenantID) + assert.Equal(t, "tenant-a", *filters.TenantID) return 1, nil }, @@ -113,6 +118,19 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { assert.Equal(t, http.StatusOK, rec.Code) }) + t.Run("empty tenant_id returns bad request", func(t *testing.T) { + mock := &mockFeedbackRecordsService{} + handler := NewFeedbackRecordsHandler(mock) + + req := httptest.NewRequestWithContext(context.Background(), + http.MethodDelete, "http://test/v1/feedback-records?user_id=user-123&tenant_id=", http.NoBody) + rec := httptest.NewRecorder() + + handler.BulkDelete(rec, req) + + assert.Equal(t, http.StatusBadRequest, rec.Code) + }) + t.Run("missing user_id returns bad request", func(t *testing.T) { mock := &mockFeedbackRecordsService{} handler := NewFeedbackRecordsHandler(mock) @@ -140,7 +158,7 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { t.Run("service error returns 500", func(t *testing.T) { mock := &mockFeedbackRecordsService{ - bulkDeleteFunc: func(_ context.Context, _ string) (int, error) { + bulkDeleteFunc: func(_ context.Context, _ *models.BulkDeleteFilters) (int, error) { return 0, assert.AnError }, } @@ -157,7 +175,7 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { t.Run("zero deleted returns 200 with deleted_count 0", func(t *testing.T) { mock := &mockFeedbackRecordsService{ - bulkDeleteFunc: func(_ context.Context, _ string) (int, error) { + bulkDeleteFunc: func(_ context.Context, _ *models.BulkDeleteFilters) (int, error) { return 0, nil }, } diff --git a/internal/models/feedback_records.go b/internal/models/feedback_records.go index 73aff65..1274910 100644 --- a/internal/models/feedback_records.go +++ b/internal/models/feedback_records.go @@ -198,7 +198,8 @@ type ListFeedbackRecordsResponse struct { // BulkDeleteFilters represents query parameters for bulk delete operation. type BulkDeleteFilters struct { - UserID string `form:"user_id" validate:"required,no_null_bytes,min=1"` + UserID string `form:"user_id" validate:"required,no_null_bytes,min=1"` + TenantID *string `form:"tenant_id" validate:"omitempty,no_null_bytes,min=1"` } // BulkDeleteResponse represents the response for bulk delete operation. diff --git a/internal/repository/feedback_records_repository.go b/internal/repository/feedback_records_repository.go index 965b766..5739813 100644 --- a/internal/repository/feedback_records_repository.go +++ b/internal/repository/feedback_records_repository.go @@ -395,16 +395,26 @@ func (r *FeedbackRecordsRepository) Delete(ctx context.Context, id uuid.UUID) er } // BulkDelete deletes all feedback records matching user_id. +// When tenant_id is provided, deletion is restricted to that tenant; otherwise all user records are deleted. // It returns deleted IDs grouped by tenant so callers can publish tenant-scoped side effects. func (r *FeedbackRecordsRepository) BulkDelete( - ctx context.Context, userID string, + ctx context.Context, filters *models.BulkDeleteFilters, ) ([]models.DeletedFeedbackRecordsByTenant, error) { query := ` DELETE FROM feedback_records - WHERE user_id = $1 + WHERE user_id = $1` + args := []any{filters.UserID} + + if filters.TenantID != nil { + query += ` AND tenant_id = $2` + + args = append(args, *filters.TenantID) + } + + query += ` RETURNING id, tenant_id` - rows, err := r.db.Query(ctx, query, userID) + rows, err := r.db.Query(ctx, query, args...) if err != nil { return nil, fmt.Errorf("failed to bulk delete feedback records: %w", err) } diff --git a/internal/repository/feedback_records_repository_test.go b/internal/repository/feedback_records_repository_test.go index a0a2653..d54efe0 100644 --- a/internal/repository/feedback_records_repository_test.go +++ b/internal/repository/feedback_records_repository_test.go @@ -6,9 +6,9 @@ import ( // BulkDelete is tested by integration tests in tests/integration_test.go: // - TestFeedbackRecordsRepository_BulkDelete exercises the repository directly and asserts -// deleted records are returned grouped by tenant. +// the optional tenant filter and tenant-grouped return values. // - TestBulkDeleteFeedbackRecords exercises the full stack (handler, service, repo) including -// GDPR user_id erasure across tenants and response shape. +// tenant-scoped deletion, GDPR user_id erasure across tenants, and response shape. func TestFeedbackRecordsRepository_Package(_ *testing.T) { // No DB in unit tests; BulkDelete coverage is in tests/. } diff --git a/internal/service/feedback_records_service.go b/internal/service/feedback_records_service.go index a3839d5..29df5c7 100644 --- a/internal/service/feedback_records_service.go +++ b/internal/service/feedback_records_service.go @@ -36,7 +36,7 @@ type FeedbackRecordsRepository interface { ) ([]models.FeedbackRecord, bool, error) Update(ctx context.Context, id uuid.UUID, req *models.UpdateFeedbackRecordRequest) (*models.FeedbackRecord, error) Delete(ctx context.Context, id uuid.UUID) error - BulkDelete(ctx context.Context, userID string) ([]models.DeletedFeedbackRecordsByTenant, error) + BulkDelete(ctx context.Context, filters *models.BulkDeleteFilters) ([]models.DeletedFeedbackRecordsByTenant, error) } // EmbeddingsRepository defines the interface for embeddings table access. @@ -205,13 +205,26 @@ func (s *FeedbackRecordsService) DeleteFeedbackRecord(ctx context.Context, id uu } // BulkDeleteFeedbackRecords deletes all feedback records matching user_id. +// When tenant_id is provided, deletion is restricted to that tenant; otherwise all user records are deleted. // It publishes one tenant-aware FeedbackRecordDeleted event per tenant represented in the deleted rows. -func (s *FeedbackRecordsService) BulkDeleteFeedbackRecords(ctx context.Context, userID string) (int, error) { - if userID == "" { +func (s *FeedbackRecordsService) BulkDeleteFeedbackRecords(ctx context.Context, filters *models.BulkDeleteFilters) (int, error) { + if filters == nil || filters.UserID == "" { return 0, ErrUserIDRequired } - groups, err := s.repo.BulkDelete(ctx, userID) + if filters.TenantID != nil { + normalizedTenantID, err := normalizeRequiredTenantID(filters.TenantID) + if err != nil { + return 0, err + } + + filters = &models.BulkDeleteFilters{ + UserID: filters.UserID, + TenantID: &normalizedTenantID, + } + } + + groups, err := s.repo.BulkDelete(ctx, filters) if err != nil { return 0, fmt.Errorf("bulk delete feedback records: %w", err) } diff --git a/internal/service/feedback_records_service_test.go b/internal/service/feedback_records_service_test.go index e5cb589..5ebd183 100644 --- a/internal/service/feedback_records_service_test.go +++ b/internal/service/feedback_records_service_test.go @@ -13,9 +13,10 @@ import ( ) type mockFeedbackRecordsRepo struct { - record *models.FeedbackRecord - bulkGroups []models.DeletedFeedbackRecordsByTenant - deletedID uuid.UUID + record *models.FeedbackRecord + bulkGroups []models.DeletedFeedbackRecordsByTenant + deletedID uuid.UUID + bulkDeleteFilters *models.BulkDeleteFilters } func (m *mockFeedbackRecordsRepo) Create( @@ -52,7 +53,11 @@ func (m *mockFeedbackRecordsRepo) Delete(_ context.Context, id uuid.UUID) error return nil } -func (m *mockFeedbackRecordsRepo) BulkDelete(_ context.Context, _ string) ([]models.DeletedFeedbackRecordsByTenant, error) { +func (m *mockFeedbackRecordsRepo) BulkDelete( + _ context.Context, filters *models.BulkDeleteFilters, +) ([]models.DeletedFeedbackRecordsByTenant, error) { + m.bulkDeleteFilters = filters + return m.bulkGroups, nil } @@ -91,11 +96,23 @@ func TestFeedbackRecordsService_BulkDeleteFeedbackRecords_PublishesTenantAwareDe publisher := &capturePublisher{} svc := NewFeedbackRecordsService(repo, nil, "", publisher, nil, "", 0) - count, err := svc.BulkDeleteFeedbackRecords(ctx, "user-123") + count, err := svc.BulkDeleteFeedbackRecords(ctx, &models.BulkDeleteFilters{UserID: "user-123"}) if err != nil { t.Fatalf("BulkDeleteFeedbackRecords() error = %v", err) } + if repo.bulkDeleteFilters == nil { + t.Fatal("repo BulkDelete filters = nil") + } + + if repo.bulkDeleteFilters.UserID != "user-123" { + t.Fatalf("repo UserID = %q, want user-123", repo.bulkDeleteFilters.UserID) + } + + if repo.bulkDeleteFilters.TenantID != nil { + t.Fatalf("repo TenantID = %q, want nil for all-tenant delete", *repo.bulkDeleteFilters.TenantID) + } + if count != len(tenantAIDs)+len(tenantBIDs) { t.Fatalf("count = %d, want %d", count, len(tenantAIDs)+len(tenantBIDs)) } @@ -104,6 +121,41 @@ func TestFeedbackRecordsService_BulkDeleteFeedbackRecords_PublishesTenantAwareDe assertDeletedEventDataAt(t, publisher, 1, datatypes.FeedbackRecordDeleted, tenantB, tenantBIDs) } +func TestFeedbackRecordsService_BulkDeleteFeedbackRecords_NormalizesTenantFilter(t *testing.T) { + ctx := context.Background() + tenantID := " org-123 " + deletedID := uuid.Must(uuid.NewV7()) + repo := &mockFeedbackRecordsRepo{ + bulkGroups: []models.DeletedFeedbackRecordsByTenant{ + {TenantID: "org-123", IDs: []uuid.UUID{deletedID}}, + }, + } + publisher := &capturePublisher{} + svc := NewFeedbackRecordsService(repo, nil, "", publisher, nil, "", 0) + + count, err := svc.BulkDeleteFeedbackRecords(ctx, &models.BulkDeleteFilters{ + UserID: "user-123", + TenantID: &tenantID, + }) + if err != nil { + t.Fatalf("BulkDeleteFeedbackRecords() error = %v", err) + } + + if count != 1 { + t.Fatalf("count = %d, want 1", count) + } + + if repo.bulkDeleteFilters == nil || repo.bulkDeleteFilters.TenantID == nil { + t.Fatal("repo TenantID = nil, want normalized tenant") + } + + if *repo.bulkDeleteFilters.TenantID != "org-123" { + t.Fatalf("repo TenantID = %q, want org-123", *repo.bulkDeleteFilters.TenantID) + } + + assertDeletedEventData(t, publisher, datatypes.FeedbackRecordDeleted, "org-123", []uuid.UUID{deletedID}) +} + func TestFeedbackRecordsService_BulkDeleteFeedbackRecords_RequiresUserID(t *testing.T) { ctx := context.Background() repo := &mockFeedbackRecordsRepo{ @@ -114,7 +166,7 @@ func TestFeedbackRecordsService_BulkDeleteFeedbackRecords_RequiresUserID(t *test publisher := &capturePublisher{} svc := NewFeedbackRecordsService(repo, nil, "", publisher, nil, "", 0) - count, err := svc.BulkDeleteFeedbackRecords(ctx, "") + count, err := svc.BulkDeleteFeedbackRecords(ctx, &models.BulkDeleteFilters{}) if !errors.Is(err, ErrUserIDRequired) { t.Fatalf("BulkDeleteFeedbackRecords() error = %v, want ErrUserIDRequired", err) } diff --git a/openapi.yaml b/openapi.yaml index 5a92ec6..e18bdf3 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -381,19 +381,28 @@ paths: tags: - Feedback Records summary: Bulk delete feedback records by user ID - description: Permanently deletes all feedback record data points matching the specified user_id across tenants. This endpoint supports GDPR Article 17 (Right to Erasure) requests. + description: Permanently deletes feedback record data points matching the specified user_id. Omit tenant_id to delete that user_id across all tenants for GDPR Article 17 (Right to Erasure) requests. Provide tenant_id to restrict deletion to that tenant only. operationId: bulk-delete-feedback-records parameters: - name: user_id in: query - description: Delete all records matching this user ID (required). NULL bytes not allowed. + description: Delete records matching this user ID (required). NULL bytes not allowed. required: true schema: type: string - description: Delete all records matching this user ID (required). NULL bytes not allowed. + description: Delete records matching this user ID (required). NULL bytes not allowed. minLength: 1 pattern: '^[^\x00]*$' example: "user-abc-123" + - name: tenant_id + in: query + description: Optional tenant scope. Omit this parameter to delete all records matching user_id across tenants; provide it to delete only records for this tenant. Empty strings and NULL bytes are not allowed. + schema: + type: string + description: Optional tenant scope. Omit this parameter to delete all records matching user_id across tenants; provide it to delete only records for this tenant. Empty strings and NULL bytes are not allowed. + minLength: 1 + pattern: '^[^\x00]*$' + example: "org-123" responses: "200": description: OK diff --git a/tests/integration_test.go b/tests/integration_test.go index ed7f850..9a6e4b6 100644 --- a/tests/integration_test.go +++ b/tests/integration_test.go @@ -885,14 +885,16 @@ func TestBulkDeleteFeedbackRecords(t *testing.T) { defer cleanup() client := &http.Client{} - userID := "bulk-delete-test-user-123" + userID := "bulk-delete-test-user-" + uuid.New().String() subID := uuid.New().String() // unique per run to avoid 409 from leftover data // Create several feedback records with the same user_id across tenants. tenantA := "test-tenant-a" tenantB := "test-tenant-b" - createPayload := func(fieldID, tenantID string, valueNum float64) map[string]any { - return map[string]any{ + createRecord := func(fieldID, tenantID string, valueNum float64) string { + t.Helper() + + body, err := json.Marshal(map[string]any{ "source_type": "formbricks", "submission_id": subID, "tenant_id": tenantID, @@ -900,38 +902,49 @@ func TestBulkDeleteFeedbackRecords(t *testing.T) { "field_id": fieldID, "field_type": "number", "value_number": valueNum, - } - } - createdIDs := make([]string, 0, 3) - - for i, p := range []map[string]any{ - createPayload("nps_1", tenantA, 8), - createPayload("nps_2", tenantB, 9), - createPayload("nps_3", tenantB, 10), - } { - body, err := json.Marshal(p) + }) require.NoError(t, err) + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, server.URL+"/v1/feedback-records", bytes.NewBuffer(body)) require.NoError(t, err) req.Header.Set("Authorization", "Bearer "+testAPIKey) req.Header.Set("Content-Type", "application/json") + resp, err := client.Do(req) require.NoError(t, err) - require.Equal(t, http.StatusCreated, resp.StatusCode, "create record %d", i+1) + require.Equal(t, http.StatusCreated, resp.StatusCode, "create record %s/%s", tenantID, fieldID) var rec models.FeedbackRecord err = decodeData(resp, &rec) require.NoError(t, err) - createdIDs = append(createdIDs, rec.ID.String()) - require.NoError(t, resp.Body.Close()) + + return rec.ID.String() + } + + requireStatus := func(id string, status int) { + t.Helper() + + getReq, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL+"/v1/feedback-records/"+id, http.NoBody) + require.NoError(t, err) + getReq.Header.Set("Authorization", "Bearer "+testAPIKey) + + getResp, err := client.Do(getReq) + require.NoError(t, err) + assert.Equal(t, status, getResp.StatusCode) + require.NoError(t, getResp.Body.Close()) } - // Bulk delete by user_id deletes every matching record for GDPR erasure, regardless of tenant. - bulkDelURL := server.URL + "/v1/feedback-records?user_id=" + userID - req, err := http.NewRequestWithContext(context.Background(), http.MethodDelete, bulkDelURL, http.NoBody) + tenantAID := createRecord("nps_1", tenantA, 8) + tenantBID1 := createRecord("nps_2", tenantB, 9) + tenantBID2 := createRecord("nps_3", tenantB, 10) + + // Providing tenant_id scopes deletion to only that tenant. + scopedDelURL := fmt.Sprintf("%s/v1/feedback-records?user_id=%s&tenant_id=%s", + server.URL, url.QueryEscape(userID), url.QueryEscape(tenantA)) + req, err := http.NewRequestWithContext(context.Background(), http.MethodDelete, scopedDelURL, http.NoBody) require.NoError(t, err) req.Header.Set("Authorization", "Bearer "+testAPIKey) @@ -939,6 +952,29 @@ func TestBulkDeleteFeedbackRecords(t *testing.T) { require.NoError(t, err) assert.Equal(t, http.StatusOK, resp.StatusCode) + var scopedResp models.BulkDeleteResponse + + err = decodeData(resp, &scopedResp) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, int64(1), scopedResp.DeletedCount) + + requireStatus(tenantAID, http.StatusNotFound) + requireStatus(tenantBID1, http.StatusOK) + requireStatus(tenantBID2, http.StatusOK) + + tenantAID2 := createRecord("nps_4", tenantA, 7) + + // Omitting tenant_id deletes every remaining matching record for GDPR erasure, regardless of tenant. + bulkDelURL := server.URL + "/v1/feedback-records?user_id=" + url.QueryEscape(userID) + req, err = http.NewRequestWithContext(context.Background(), http.MethodDelete, bulkDelURL, http.NoBody) + require.NoError(t, err) + req.Header.Set("Authorization", "Bearer "+testAPIKey) + + resp, err = client.Do(req) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + var bulkResp models.BulkDeleteResponse err = decodeData(resp, &bulkResp) @@ -948,18 +984,12 @@ func TestBulkDeleteFeedbackRecords(t *testing.T) { assert.Equal(t, "Successfully deleted 3 feedback records", bulkResp.Message) // Verify records are gone - for _, id := range createdIDs { - getReq, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL+"/v1/feedback-records/"+id, http.NoBody) - require.NoError(t, err) - getReq.Header.Set("Authorization", "Bearer "+testAPIKey) - getResp, err := client.Do(getReq) - require.NoError(t, err) - assert.Equal(t, http.StatusNotFound, getResp.StatusCode) - require.NoError(t, getResp.Body.Close()) + for _, id := range []string{tenantAID2, tenantBID1, tenantBID2} { + requireStatus(id, http.StatusNotFound) } // Bulk delete again (no matching records) returns 0 - bulkDelURL2 := server.URL + "/v1/feedback-records?user_id=" + userID + bulkDelURL2 := server.URL + "/v1/feedback-records?user_id=" + url.QueryEscape(userID) req2, err := http.NewRequestWithContext(context.Background(), http.MethodDelete, bulkDelURL2, http.NoBody) require.NoError(t, err) req2.Header.Set("Authorization", "Bearer "+testAPIKey) @@ -997,7 +1027,7 @@ func TestFeedbackRecordsRepository_BulkDelete(t *testing.T) { defer db.Close() repo := repository.NewFeedbackRecordsRepository(db) - userID := "repo-bulk-delete-user" + userID := "repo-bulk-delete-user-" + uuid.New().String() sourceType := "formbricks" // Create records with same user_id across tenants. @@ -1045,22 +1075,34 @@ func TestFeedbackRecordsRepository_BulkDelete(t *testing.T) { require.NoError(t, err) require.NotEmpty(t, rec3.ID) - // BulkDelete returns deleted IDs grouped by tenant for tenant-safe side effects. - deletedGroups, err := repo.BulkDelete(ctx, userID) + // BulkDelete with tenant_id restricts deletion to that tenant and returns tenant-safe groups. + tenantFilter := bulkDeleteTenant + deletedGroups, err := repo.BulkDelete(ctx, &models.BulkDeleteFilters{ + UserID: userID, + TenantID: &tenantFilter, + }) require.NoError(t, err) - require.Len(t, deletedGroups, 2) + require.Len(t, deletedGroups, 1) + require.Equal(t, bulkDeleteTenant, deletedGroups[0].TenantID) + assert.ElementsMatch(t, []uuid.UUID{rec1.ID, rec2.ID}, deletedGroups[0].IDs) - idsByTenant := map[string]map[string]bool{} - for _, group := range deletedGroups { - idsByTenant[group.TenantID] = map[string]bool{} - for _, id := range group.IDs { - idsByTenant[group.TenantID][id.String()] = true - } - } + _, err = repo.GetByID(ctx, rec1.ID) + require.Error(t, err) + _, err = repo.GetByID(ctx, rec2.ID) + require.Error(t, err) + remaining, err := repo.GetByID(ctx, rec3.ID) + require.NoError(t, err) + require.Equal(t, otherBulkDeleteTenant, remaining.TenantID) + + // Omitting tenant_id deletes the rest of the user records across tenants. + deletedGroups, err = repo.BulkDelete(ctx, &models.BulkDeleteFilters{UserID: userID}) + require.NoError(t, err) + require.Len(t, deletedGroups, 1) + require.Equal(t, otherBulkDeleteTenant, deletedGroups[0].TenantID) + assert.ElementsMatch(t, []uuid.UUID{rec3.ID}, deletedGroups[0].IDs) - assert.True(t, idsByTenant[bulkDeleteTenant][rec1.ID.String()]) - assert.True(t, idsByTenant[bulkDeleteTenant][rec2.ID.String()]) - assert.True(t, idsByTenant[otherBulkDeleteTenant][rec3.ID.String()]) + _, err = repo.GetByID(ctx, rec3.ID) + require.Error(t, err) } // TestWebhooksCRUD tests webhook create, get, list, update, delete. From 8bc03cc90154db1f93a8bb9714e5e8ca561805c0 Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Tue, 28 Apr 2026 22:36:08 +0000 Subject: [PATCH 09/12] chore: add gdpr exception --- AGENTS.md | 1 + openapi.yaml | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c05ec8e..4b7949d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,6 +28,7 @@ ## Multi-Tenancy & Data Isolation - Treat `tenant_id` as a security boundary, not a convenience filter. Tenant-owned data must never be read, enqueued, dispatched, cached, searched, embedded, or deleted across tenants. +- Exception: GDPR/right-to-erasure flows may intentionally delete all records for a data subject across tenants when that is the documented API contract. Make that all-tenant behavior explicit in the API docs, service/repository names or comments, logs, and tests; do not reuse it for normal tenant-owned workflows. - When making a model, migration, API request, or repository change involving tenant-owned data, audit every downstream path that carries or derives from that data: handlers, services, repositories, message publishers, River job args, workers, webhook payloads, search, embeddings, bulk operations, logs, and metrics. - Tenant access rules must be consistent across every path that can observe, mutate, derive from, or act on the same resource. If one API endpoint, repository method, search path, webhook dispatch path, worker, backfill, bulk operation, or export path requires tenant scope, every alternate path for that resource must enforce the same tenant boundary. - Do not model `tenant_id` as an optional filter for tenant-owned resources. Prefer required tenant parameters in service/repository method signatures (`tenantID string`, not `*string`) unless the domain explicitly supports global resources and documents that behavior. diff --git a/openapi.yaml b/openapi.yaml index e18bdf3..1dc868d 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -1550,7 +1550,6 @@ components: - id - url - enabled - - tenant_id - created_at - updated_at WebhookData: @@ -1598,7 +1597,6 @@ components: - url - signing_key - enabled - - tenant_id - created_at - updated_at WebhookEventType: From fd5183d0465d2ff3e0d1aa68ec18d3d9bec61f8d Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Wed, 29 Apr 2026 10:04:25 +0000 Subject: [PATCH 10/12] chore: address pr comments --- .../api/handlers/feedback_records_handler.go | 6 ++ .../handlers/feedback_records_handler_test.go | 97 ++++++++++++++++++- internal/models/webhooks.go | 6 ++ internal/repository/webhooks_repository.go | 24 +++-- internal/service/feedback_records_service.go | 10 +- .../service/feedback_records_service_test.go | 47 ++++++++- internal/service/tenant_validation.go | 6 +- internal/service/webhooks_service.go | 8 +- internal/service/webhooks_service_test.go | 20 ++-- internal/workers/webhook_dispatch.go | 13 +++ internal/workers/webhook_dispatch_test.go | 28 ++++++ tests/webhooks_repository_test.go | 48 +++++++++ 12 files changed, 287 insertions(+), 26 deletions(-) diff --git a/internal/api/handlers/feedback_records_handler.go b/internal/api/handlers/feedback_records_handler.go index 90edc20..2979e82 100644 --- a/internal/api/handlers/feedback_records_handler.go +++ b/internal/api/handlers/feedback_records_handler.go @@ -60,6 +60,12 @@ func (h *FeedbackRecordsHandler) Create(w http.ResponseWriter, r *http.Request) record, err := h.service.CreateFeedbackRecord(r.Context(), &req) if err != nil { + if errors.Is(err, huberrors.ErrValidation) { + validation.RespondValidationError(w, err) + + return + } + if errors.Is(err, huberrors.ErrNotFound) { response.RespondNotFound(w, "Feedback record not found") diff --git a/internal/api/handlers/feedback_records_handler_test.go b/internal/api/handlers/feedback_records_handler_test.go index a2e8758..205dbc9 100644 --- a/internal/api/handlers/feedback_records_handler_test.go +++ b/internal/api/handlers/feedback_records_handler_test.go @@ -1,6 +1,7 @@ package handlers import ( + "bytes" "context" "encoding/json" "net/http" @@ -11,17 +12,23 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/formbricks/hub/internal/huberrors" "github.com/formbricks/hub/internal/models" ) // mockFeedbackRecordsService mocks FeedbackRecordsService for handler tests. type mockFeedbackRecordsService struct { + createFunc func(ctx context.Context, req *models.CreateFeedbackRecordRequest) (*models.FeedbackRecord, error) bulkDeleteFunc func(ctx context.Context, filters *models.BulkDeleteFilters) (int, error) } func (m *mockFeedbackRecordsService) CreateFeedbackRecord( - context.Context, *models.CreateFeedbackRecordRequest, + ctx context.Context, req *models.CreateFeedbackRecordRequest, ) (*models.FeedbackRecord, error) { + if m.createFunc != nil { + return m.createFunc(ctx, req) + } + return nil, nil } @@ -69,6 +76,94 @@ func TestFeedbackRecordsHandler_List(t *testing.T) { }) } +func TestFeedbackRecordsHandler_Create(t *testing.T) { + t.Run("success returns created record", func(t *testing.T) { + recordID := uuid.Must(uuid.NewV7()) + mock := &mockFeedbackRecordsService{ + createFunc: func(_ context.Context, req *models.CreateFeedbackRecordRequest) (*models.FeedbackRecord, error) { + assert.Equal(t, "org-123", req.TenantID) + + return &models.FeedbackRecord{ + ID: recordID, + SourceType: req.SourceType, + FieldID: req.FieldID, + FieldType: req.FieldType, + TenantID: req.TenantID, + SubmissionID: req.SubmissionID, + }, nil + }, + } + handler := NewFeedbackRecordsHandler(mock) + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodPost, "http://test/v1/feedback-records", feedbackRecordCreateBody(t, "org-123"), + ) + rec := httptest.NewRecorder() + + handler.Create(rec, req) + + assert.Equal(t, http.StatusCreated, rec.Code) + + var got models.FeedbackRecord + err := json.Unmarshal(rec.Body.Bytes(), &got) + require.NoError(t, err) + assert.Equal(t, recordID, got.ID) + assert.Equal(t, "org-123", got.TenantID) + }) + + t.Run("service validation error returns bad request", func(t *testing.T) { + mock := &mockFeedbackRecordsService{ + createFunc: func(_ context.Context, _ *models.CreateFeedbackRecordRequest) (*models.FeedbackRecord, error) { + return nil, huberrors.NewValidationError("tenant_id", "tenant_id is required and cannot be empty") + }, + } + handler := NewFeedbackRecordsHandler(mock) + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodPost, "http://test/v1/feedback-records", feedbackRecordCreateBody(t, " "), + ) + rec := httptest.NewRecorder() + + handler.Create(rec, req) + + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Contains(t, rec.Header().Get("Content-Type"), "application/problem+json") + }) + + t.Run("service conflict returns conflict", func(t *testing.T) { + mock := &mockFeedbackRecordsService{ + createFunc: func(_ context.Context, _ *models.CreateFeedbackRecordRequest) (*models.FeedbackRecord, error) { + return nil, huberrors.NewConflictError("duplicate feedback record") + }, + } + handler := NewFeedbackRecordsHandler(mock) + + req := httptest.NewRequestWithContext( + context.Background(), http.MethodPost, "http://test/v1/feedback-records", feedbackRecordCreateBody(t, "org-123"), + ) + rec := httptest.NewRecorder() + + handler.Create(rec, req) + + assert.Equal(t, http.StatusConflict, rec.Code) + }) +} + +func feedbackRecordCreateBody(t *testing.T, tenantID string) *bytes.Reader { + t.Helper() + + body, err := json.Marshal(map[string]any{ + "source_type": "formbricks", + "submission_id": "submission-1", + "tenant_id": tenantID, + "field_id": "feedback", + "field_type": "text", + }) + require.NoError(t, err) + + return bytes.NewReader(body) +} + func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) { t.Run("success returns 200 with deleted_count and message", func(t *testing.T) { mock := &mockFeedbackRecordsService{ diff --git a/internal/models/webhooks.go b/internal/models/webhooks.go index 5f2931d..32f6829 100644 --- a/internal/models/webhooks.go +++ b/internal/models/webhooks.go @@ -34,6 +34,12 @@ type Webhook struct { DisabledAt *time.Time `json:"disabled_at,omitempty"` } +// DeletedWebhook is the minimal data returned after deleting a webhook. +type DeletedWebhook struct { + ID uuid.UUID + TenantID *string +} + // MarshalJSON converts []datatypes.EventType to JSON string array. func (w *Webhook) MarshalJSON() ([]byte, error) { type Alias Webhook diff --git a/internal/repository/webhooks_repository.go b/internal/repository/webhooks_repository.go index 67d3793..2a32a13 100644 --- a/internal/repository/webhooks_repository.go +++ b/internal/repository/webhooks_repository.go @@ -333,20 +333,26 @@ func (r *WebhooksRepository) Update(ctx context.Context, id uuid.UUID, req *mode return &webhook, nil } -// Delete removes a webhook. -func (r *WebhooksRepository) Delete(ctx context.Context, id uuid.UUID) error { - query := `DELETE FROM webhooks WHERE id = $1` +// Delete removes a webhook and returns the deleted tenant boundary for side effects. +func (r *WebhooksRepository) Delete(ctx context.Context, id uuid.UUID) (*models.DeletedWebhook, error) { + query := ` + DELETE FROM webhooks + WHERE id = $1 + RETURNING id, tenant_id + ` - result, err := r.db.Exec(ctx, query, id) + var webhook models.DeletedWebhook + + err := r.db.QueryRow(ctx, query, id).Scan(&webhook.ID, &webhook.TenantID) if err != nil { - return fmt.Errorf("failed to delete webhook: %w", err) - } + if errors.Is(err, pgx.ErrNoRows) { + return nil, huberrors.NewNotFoundError("webhook", "webhook not found") + } - if result.RowsAffected() == 0 { - return huberrors.NewNotFoundError("webhook", "webhook not found") + return nil, fmt.Errorf("failed to delete webhook: %w", err) } - return nil + return &webhook, nil } // parseDBEventTypes converts a DB string slice to []datatypes.EventType. Returns (nil, nil) for nil input. diff --git a/internal/service/feedback_records_service.go b/internal/service/feedback_records_service.go index 29df5c7..61b792e 100644 --- a/internal/service/feedback_records_service.go +++ b/internal/service/feedback_records_service.go @@ -92,7 +92,15 @@ func (s *FeedbackRecordsService) SetEmbeddingInserter(inserter FeedbackEmbedding func (s *FeedbackRecordsService) CreateFeedbackRecord( ctx context.Context, req *models.CreateFeedbackRecordRequest, ) (*models.FeedbackRecord, error) { - record, err := s.repo.Create(ctx, req) + normalizedTenantID, err := normalizeRequiredTenantIDValue(req.TenantID) + if err != nil { + return nil, err + } + + normalizedReq := *req + normalizedReq.TenantID = normalizedTenantID + + record, err := s.repo.Create(ctx, &normalizedReq) if err != nil { return nil, fmt.Errorf("create feedback record: %w", err) } diff --git a/internal/service/feedback_records_service_test.go b/internal/service/feedback_records_service_test.go index 5ebd183..13ad699 100644 --- a/internal/service/feedback_records_service_test.go +++ b/internal/service/feedback_records_service_test.go @@ -14,15 +14,23 @@ import ( type mockFeedbackRecordsRepo struct { record *models.FeedbackRecord + createReq *models.CreateFeedbackRecordRequest bulkGroups []models.DeletedFeedbackRecordsByTenant deletedID uuid.UUID bulkDeleteFilters *models.BulkDeleteFilters } func (m *mockFeedbackRecordsRepo) Create( - _ context.Context, _ *models.CreateFeedbackRecordRequest, + _ context.Context, req *models.CreateFeedbackRecordRequest, ) (*models.FeedbackRecord, error) { - return nil, errors.New("not implemented") + reqCopy := *req + m.createReq = &reqCopy + + if m.record != nil { + return m.record, nil + } + + return &models.FeedbackRecord{TenantID: req.TenantID}, nil } func (m *mockFeedbackRecordsRepo) GetByID(_ context.Context, _ uuid.UUID) (*models.FeedbackRecord, error) { @@ -81,6 +89,41 @@ func TestFeedbackRecordsService_DeleteFeedbackRecord_PublishesTenantAwareDeleted assertDeletedEventData(t, publisher, datatypes.FeedbackRecordDeleted, tenantID, []uuid.UUID{recordID}) } +func TestFeedbackRecordsService_CreateFeedbackRecord_NormalizesTenantID(t *testing.T) { + ctx := context.Background() + inputTenantID := " org-123 " + repo := &mockFeedbackRecordsRepo{} + publisher := &capturePublisher{} + svc := NewFeedbackRecordsService(repo, nil, "", publisher, nil, "", 0) + + record, err := svc.CreateFeedbackRecord(ctx, &models.CreateFeedbackRecordRequest{ + SourceType: "formbricks", + FieldID: "field-1", + FieldType: models.FieldTypeText, + TenantID: inputTenantID, + SubmissionID: "submission-1", + }) + if err != nil { + t.Fatalf("CreateFeedbackRecord() error = %v", err) + } + + if repo.createReq == nil { + t.Fatal("repo Create request = nil") + } + + if repo.createReq.TenantID != "org-123" { + t.Fatalf("repo TenantID = %q, want org-123", repo.createReq.TenantID) + } + + if record.TenantID != "org-123" { + t.Fatalf("record TenantID = %q, want org-123", record.TenantID) + } + + if publisher.callCount != 1 || publisher.eventType != datatypes.FeedbackRecordCreated { + t.Fatalf("published event = (%d, %s), want one feedback_record.created", publisher.callCount, publisher.eventType) + } +} + func TestFeedbackRecordsService_BulkDeleteFeedbackRecords_PublishesTenantAwareDeletedEventsByTenant(t *testing.T) { ctx := context.Background() tenantA := "org-123" diff --git a/internal/service/tenant_validation.go b/internal/service/tenant_validation.go index 44af4fb..0ecf632 100644 --- a/internal/service/tenant_validation.go +++ b/internal/service/tenant_validation.go @@ -11,7 +11,11 @@ func normalizeRequiredTenantID(tenantID *string) (string, error) { return "", huberrors.NewValidationError("tenant_id", "tenant_id is required") } - normalized := strings.TrimSpace(*tenantID) + return normalizeRequiredTenantIDValue(*tenantID) +} + +func normalizeRequiredTenantIDValue(tenantID string) (string, error) { + normalized := strings.TrimSpace(tenantID) if normalized == "" { return "", huberrors.NewValidationError("tenant_id", "tenant_id is required and cannot be empty") } diff --git a/internal/service/webhooks_service.go b/internal/service/webhooks_service.go index 4549bd5..a249341 100644 --- a/internal/service/webhooks_service.go +++ b/internal/service/webhooks_service.go @@ -32,7 +32,7 @@ type WebhooksRepository interface { ) ([]models.Webhook, bool, error) Count(ctx context.Context, filters *models.ListWebhooksFilters) (int64, error) Update(ctx context.Context, id uuid.UUID, req *models.UpdateWebhookRequest) (*models.Webhook, error) - Delete(ctx context.Context, id uuid.UUID) error + Delete(ctx context.Context, id uuid.UUID) (*models.DeletedWebhook, error) } // WebhooksService handles business logic for webhooks. @@ -409,12 +409,8 @@ func normalizeWebhookTenantID(tenantID *string) error { // DeleteWebhook deletes a webhook by ID. // Publishes WebhookDeleted with tenant-aware deleted IDs. func (s *WebhooksService) DeleteWebhook(ctx context.Context, id uuid.UUID) error { - webhook, err := s.repo.GetByID(ctx, id) + webhook, err := s.repo.Delete(ctx, id) if err != nil { - return fmt.Errorf("get webhook before delete: %w", err) - } - - if err := s.repo.Delete(ctx, id); err != nil { return fmt.Errorf("delete webhook: %w", err) } diff --git a/internal/service/webhooks_service_test.go b/internal/service/webhooks_service_test.go index 95305a3..f29f58b 100644 --- a/internal/service/webhooks_service_test.go +++ b/internal/service/webhooks_service_test.go @@ -15,9 +15,11 @@ import ( ) type mockWebhooksRepo struct { - count int64 - webhook *models.Webhook - deletedID uuid.UUID + count int64 + webhook *models.Webhook + deleted *models.DeletedWebhook + deletedID uuid.UUID + getByIDCalls int } func (m *mockWebhooksRepo) Create(_ context.Context, _ *models.CreateWebhookRequest) (*models.Webhook, error) { @@ -25,6 +27,8 @@ func (m *mockWebhooksRepo) Create(_ context.Context, _ *models.CreateWebhookRequ } func (m *mockWebhooksRepo) GetByID(_ context.Context, _ uuid.UUID) (*models.Webhook, error) { + m.getByIDCalls++ + if m.webhook != nil { return m.webhook, nil } @@ -50,10 +54,10 @@ func (m *mockWebhooksRepo) Update(_ context.Context, _ uuid.UUID, _ *models.Upda return nil, nil } -func (m *mockWebhooksRepo) Delete(_ context.Context, id uuid.UUID) error { +func (m *mockWebhooksRepo) Delete(_ context.Context, id uuid.UUID) (*models.DeletedWebhook, error) { m.deletedID = id - return nil + return m.deleted, nil } type noopPublisher struct{} @@ -210,7 +214,7 @@ func TestWebhooksService_DeleteWebhook_PublishesTenantAwareDeletedEvent(t *testi ctx := context.Background() webhookID := uuid.Must(uuid.NewV7()) tenantID := "org-123" - repo := &mockWebhooksRepo{webhook: &models.Webhook{ID: webhookID, TenantID: &tenantID}} + repo := &mockWebhooksRepo{deleted: &models.DeletedWebhook{ID: webhookID, TenantID: &tenantID}} publisher := &capturePublisher{} svc := NewWebhooksService(repo, publisher, 10, nil) @@ -223,6 +227,10 @@ func TestWebhooksService_DeleteWebhook_PublishesTenantAwareDeletedEvent(t *testi t.Fatalf("deletedID = %v, want %v", repo.deletedID, webhookID) } + if repo.getByIDCalls != 0 { + t.Fatalf("GetByID called %d times, want 0; delete should return the deleted row atomically", repo.getByIDCalls) + } + if publisher.callCount != 1 || publisher.eventType != datatypes.WebhookDeleted { t.Fatalf("published event = (%d, %s), want one webhook.deleted", publisher.callCount, publisher.eventType) } diff --git a/internal/workers/webhook_dispatch.go b/internal/workers/webhook_dispatch.go index 1fa7072..d45ac63 100644 --- a/internal/workers/webhook_dispatch.go +++ b/internal/workers/webhook_dispatch.go @@ -107,6 +107,19 @@ func (w *WebhookDispatchWorker) Work(ctx context.Context, job *river.Job[service tenantID = dataTenantID } + if tenantID == nil { + if w.metrics != nil { + w.metrics.RecordDispatchError(ctx, "missing_tenant_id") + } + + slog.Error("webhook dispatch: event tenant_id missing, skipping delivery", + "event_id", args.EventID, + "webhook_id", args.WebhookID, + ) + + return nil + } + if !service.WebhookMatchesTenant(webhook, tenantID) { if w.metrics != nil { w.metrics.RecordDispatchError(ctx, "tenant_mismatch") diff --git a/internal/workers/webhook_dispatch_test.go b/internal/workers/webhook_dispatch_test.go index 0b20701..72fc72f 100644 --- a/internal/workers/webhook_dispatch_test.go +++ b/internal/workers/webhook_dispatch_test.go @@ -162,6 +162,34 @@ func TestWebhookDispatchWorker_Work(t *testing.T) { } }) + t.Run("returns nil without send when job has no tenant boundary", func(t *testing.T) { + webhookTenant := "org-123" + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ + ID: webhookID, + Enabled: true, + URL: "http://x", + SigningKey: "sk", + TenantID: &webhookTenant, + }, + } + sender := &mockSender{} + worker := NewWebhookDispatchWorker(repo, sender, 15*time.Second, nil) + scopedArgs := args + scopedArgs.Data = nil + scopedArgs.TenantID = nil + job := &river.Job[service.WebhookDispatchArgs]{JobRow: &rivertype.JobRow{}, Args: scopedArgs} + + err := worker.Work(ctx, job) + if err != nil { + t.Errorf("Work() error = %v, want nil", err) + } + + if sender.calls != 0 { + t.Errorf("Send called %d times, want 0", sender.calls) + } + }) + t.Run("returns nil without send when job tenant conflicts with payload tenant", func(t *testing.T) { payloadTenant := "org-other" repo := &mockDispatchRepo{ diff --git a/tests/webhooks_repository_test.go b/tests/webhooks_repository_test.go index 6459891..45fead7 100644 --- a/tests/webhooks_repository_test.go +++ b/tests/webhooks_repository_test.go @@ -12,6 +12,7 @@ import ( "github.com/formbricks/hub/internal/config" "github.com/formbricks/hub/internal/datatypes" + "github.com/formbricks/hub/internal/huberrors" "github.com/formbricks/hub/internal/models" "github.com/formbricks/hub/internal/repository" "github.com/formbricks/hub/pkg/database" @@ -79,6 +80,53 @@ func TestWebhooksRepository_ListEnabledForEventTypeAndTenant(t *testing.T) { assertRepositoryScopeWebhookIDs(t, tenantlessWebhooks, urlPrefix, map[uuid.UUID]bool{}) } +func TestWebhooksRepository_DeleteReturnsDeletedWebhook(t *testing.T) { + ctx := context.Background() + urlPrefix := "https://tenant-delete.test/" + uuid.NewString() + "/" + + databaseURL := os.Getenv("DATABASE_URL") + if databaseURL == "" { + databaseURL = defaultTestDatabaseURL + } + + t.Setenv("API_KEY", testAPIKey) + t.Setenv("DATABASE_URL", databaseURL) + + cfg, err := config.Load() + require.NoError(t, err) + + db, err := database.NewPostgresPool(ctx, cfg.Database.URL, + database.WithPoolConfig(cfg.Database.PoolConfig()), + ) + require.NoError(t, err) + + defer db.Close() + + cleanupRepositoryWebhookDeleteTestRows := func() { + _, cleanupErr := db.Exec(ctx, "DELETE FROM webhooks WHERE url LIKE $1", urlPrefix+"%") + require.NoError(t, cleanupErr) + } + + cleanupRepositoryWebhookDeleteTestRows() + defer cleanupRepositoryWebhookDeleteTestRows() + + repo := repository.NewWebhooksRepository(db) + tenantID := "repo-delete-tenant" + webhook := createWebhookForRepositoryScopeTest( + ctx, t, repo, urlPrefix, "delete-returning", &tenantID, []datatypes.EventType{datatypes.WebhookDeleted}, + ) + + deleted, err := repo.Delete(ctx, webhook.ID) + require.NoError(t, err) + require.NotNil(t, deleted) + require.NotNil(t, deleted.TenantID) + assert.Equal(t, webhook.ID, deleted.ID) + assert.Equal(t, tenantID, *deleted.TenantID) + + _, err = repo.GetByID(ctx, webhook.ID) + assert.ErrorIs(t, err, huberrors.ErrNotFound) +} + func createWebhookForRepositoryScopeTest( ctx context.Context, t *testing.T, From 754e4fb4e49743055ba7613ea0d5fd3605b63ddc Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Wed, 29 Apr 2026 10:07:31 +0000 Subject: [PATCH 11/12] chore: lint fix --- internal/api/handlers/feedback_records_handler_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/api/handlers/feedback_records_handler_test.go b/internal/api/handlers/feedback_records_handler_test.go index 205dbc9..ab05899 100644 --- a/internal/api/handlers/feedback_records_handler_test.go +++ b/internal/api/handlers/feedback_records_handler_test.go @@ -105,6 +105,7 @@ func TestFeedbackRecordsHandler_Create(t *testing.T) { assert.Equal(t, http.StatusCreated, rec.Code) var got models.FeedbackRecord + err := json.Unmarshal(rec.Body.Bytes(), &got) require.NoError(t, err) assert.Equal(t, recordID, got.ID) From ed966af8cef98705da12727bcfcbc2d0ea249087 Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Wed, 29 Apr 2026 10:43:56 +0000 Subject: [PATCH 12/12] fix: harden webhook delete payload dispatch --- internal/observability/names.go | 1 + internal/observability/names_test.go | 21 ++++ internal/service/webhook_payload.go | 120 +++++++++++++++++++++-- internal/service/webhook_payload_test.go | 66 +++++++++++++ 4 files changed, 202 insertions(+), 6 deletions(-) create mode 100644 internal/observability/names_test.go diff --git a/internal/observability/names.go b/internal/observability/names.go index 6e0e750..cb16de8 100644 --- a/internal/observability/names.go +++ b/internal/observability/names.go @@ -64,6 +64,7 @@ var allowedDisabledReasons = map[string]bool{ // allowedDispatchReasons for hub_webhook_dispatch_errors_total. var allowedDispatchReasons = map[string]bool{ "get_webhook_failed": true, + "missing_tenant_id": true, "tenant_mismatch": true, } diff --git a/internal/observability/names_test.go b/internal/observability/names_test.go new file mode 100644 index 0000000..7300939 --- /dev/null +++ b/internal/observability/names_test.go @@ -0,0 +1,21 @@ +package observability + +import "testing" + +func TestAllowedDispatchReasonIncludesTenantBoundaryReasons(t *testing.T) { + reasons := []string{ + "get_webhook_failed", + "missing_tenant_id", + "tenant_mismatch", + } + + for _, reason := range reasons { + if !AllowedDispatchReason(reason) { + t.Errorf("AllowedDispatchReason(%q) = false, want true", reason) + } + + if got := NormalizeReason(reason, AllowedDispatchReason); got != reason { + t.Errorf("NormalizeReason(%q) = %q, want %q", reason, got, reason) + } + } +} diff --git a/internal/service/webhook_payload.go b/internal/service/webhook_payload.go index 20df651..d070ae2 100644 --- a/internal/service/webhook_payload.go +++ b/internal/service/webhook_payload.go @@ -1,10 +1,12 @@ package service import ( + "encoding/json" "time" "github.com/google/uuid" + "github.com/formbricks/hub/internal/datatypes" "github.com/formbricks/hub/internal/models" ) @@ -31,23 +33,55 @@ func NewWebhookPayload(args WebhookDispatchArgs) *WebhookPayload { Type: args.EventType, Timestamp: args.Timestamp, TenantID: tenantID, - Data: publicWebhookData(args.Data), + Data: publicWebhookData(args.EventType, args.Data), ChangedFields: args.ChangedFields, } } -func publicWebhookData(data any) any { +func publicWebhookData(eventType string, data any) any { + if !isDeletedIDsEvent(eventType) { + return data + } + + ids, ok := deletedIDsFromEventData(data) + if !ok { + return data + } + + return ids +} + +func isDeletedIDsEvent(eventType string) bool { + return eventType == datatypes.FeedbackRecordDeleted.String() || + eventType == datatypes.WebhookDeleted.String() +} + +func deletedIDsFromEventData(data any) ([]uuid.UUID, bool) { switch payload := data.(type) { case models.DeletedIDsEventData: - return cloneUUIDs(payload.IDs) + return cloneUUIDs(payload.IDs), true case *models.DeletedIDsEventData: if payload == nil { - return nil + return nil, true } - return cloneUUIDs(payload.IDs) + return cloneUUIDs(payload.IDs), true + case map[string]any: + return deletedIDsFromValue(payload["ids"]) + case map[string][]uuid.UUID: + return cloneUUIDs(payload["ids"]), true + case map[string][]string: + return deletedIDsFromStrings(payload["ids"]) + case []uuid.UUID: + return cloneUUIDs(payload), true + case []string: + return deletedIDsFromStrings(payload) + case []any: + return deletedIDsFromValues(payload) + case json.RawMessage: + return deletedIDsFromRawJSON(payload) default: - return data + return deletedIDsFromJSON(data) } } @@ -66,3 +100,77 @@ func cloneUUIDs(ids []uuid.UUID) []uuid.UUID { return append([]uuid.UUID(nil), ids...) } + +func deletedIDsFromValue(value any) ([]uuid.UUID, bool) { + switch ids := value.(type) { + case []uuid.UUID: + return cloneUUIDs(ids), true + case []string: + return deletedIDsFromStrings(ids) + case []any: + return deletedIDsFromValues(ids) + default: + return nil, false + } +} + +func deletedIDsFromStrings(values []string) ([]uuid.UUID, bool) { + ids := make([]uuid.UUID, 0, len(values)) + for _, value := range values { + id, err := uuid.Parse(value) + if err != nil { + return nil, false + } + + ids = append(ids, id) + } + + return ids, true +} + +func deletedIDsFromValues(values []any) ([]uuid.UUID, bool) { + ids := make([]uuid.UUID, 0, len(values)) + for _, value := range values { + switch id := value.(type) { + case uuid.UUID: + ids = append(ids, id) + case string: + parsed, err := uuid.Parse(id) + if err != nil { + return nil, false + } + + ids = append(ids, parsed) + default: + return nil, false + } + } + + return ids, true +} + +func deletedIDsFromJSON(data any) ([]uuid.UUID, bool) { + payload, err := json.Marshal(data) + if err != nil { + return nil, false + } + + return deletedIDsFromRawJSON(payload) +} + +func deletedIDsFromRawJSON(payload []byte) ([]uuid.UUID, bool) { + var envelope struct { + IDs []uuid.UUID `json:"ids"` + } + + if err := json.Unmarshal(payload, &envelope); err == nil && envelope.IDs != nil { + return cloneUUIDs(envelope.IDs), true + } + + var ids []uuid.UUID + if err := json.Unmarshal(payload, &ids); err != nil { + return nil, false + } + + return cloneUUIDs(ids), true +} diff --git a/internal/service/webhook_payload_test.go b/internal/service/webhook_payload_test.go index 0bb5d82..3c5db91 100644 --- a/internal/service/webhook_payload_test.go +++ b/internal/service/webhook_payload_test.go @@ -1,6 +1,7 @@ package service import ( + "encoding/json" "testing" "time" @@ -48,6 +49,52 @@ func TestNewWebhookPayload_MapsDeletedIDsEventDataToPublicPayload(t *testing.T) } } +func TestNewWebhookPayload_MapsJSONRoundTrippedDeletedIDsEventDataToPublicPayload(t *testing.T) { + tenantID := "org-123" + ids := []uuid.UUID{uuid.Must(uuid.NewV7()), uuid.Must(uuid.NewV7())} + args := WebhookDispatchArgs{ + EventID: uuid.Must(uuid.NewV7()), + EventType: "feedback_record.deleted", + Timestamp: time.Now(), + Data: map[string]any{ + "tenant_id": tenantID, + "ids": []any{ids[0].String(), ids[1].String()}, + }, + WebhookID: uuid.Must(uuid.NewV7()), + } + + payload := NewWebhookPayload(args) + + if payload.TenantID == nil || *payload.TenantID != tenantID { + t.Fatalf("TenantID = %v, want %q", payload.TenantID, tenantID) + } + + assertWebhookPayloadIDs(t, payload.Data, ids) +} + +func TestNewWebhookPayload_MapsRawJSONDeletedIDsEventDataToPublicPayload(t *testing.T) { + tenantID := "org-123" + ids := []uuid.UUID{uuid.Must(uuid.NewV7()), uuid.Must(uuid.NewV7())} + args := WebhookDispatchArgs{ + EventID: uuid.Must(uuid.NewV7()), + EventType: "webhook.deleted", + Timestamp: time.Now(), + Data: json.RawMessage(`{ + "tenant_id": "` + tenantID + `", + "ids": ["` + ids[0].String() + `", "` + ids[1].String() + `"] + }`), + WebhookID: uuid.Must(uuid.NewV7()), + } + + payload := NewWebhookPayload(args) + + if payload.TenantID == nil || *payload.TenantID != tenantID { + t.Fatalf("TenantID = %v, want %q", payload.TenantID, tenantID) + } + + assertWebhookPayloadIDs(t, payload.Data, ids) +} + func TestNewWebhookPayload_DerivesTenantFromLegacyArgsData(t *testing.T) { tenantID := "org-123" args := WebhookDispatchArgs{ @@ -64,3 +111,22 @@ func TestNewWebhookPayload_DerivesTenantFromLegacyArgsData(t *testing.T) { t.Fatalf("TenantID = %v, want %q", payload.TenantID, tenantID) } } + +func assertWebhookPayloadIDs(t *testing.T, data any, want []uuid.UUID) { + t.Helper() + + got, ok := data.([]uuid.UUID) + if !ok { + t.Fatalf("Data type = %T, want []uuid.UUID", data) + } + + if len(got) != len(want) { + t.Fatalf("Data length = %d, want %d", len(got), len(want)) + } + + for i := range want { + if got[i] != want[i] { + t.Errorf("Data[%d] = %v, want %v", i, got[i], want[i]) + } + } +}