Name a deleted target on its historical deliveries (closes #211)

internal/delivery/target_config_view.go

@@ -23,6 +23,13 @@ type ConfigField struct {
 	Value string
 }
 
+// deletedNameSuffix marks the name of a target that no longer
+// exists. Deletes are soft and delivery history outlives the
+// target, so the event log shows names of targets that are gone;
+// an operator reading one needs to know it cannot be delivered
+// to, replayed to, or configured.
+const deletedNameSuffix = " (deleted)"
+
 // TargetView is the display-safe projection of a target for
 // the UI. It deliberately has no raw configuration field, so
 // no template — present or future — can render the stored
@@ -30,14 +37,37 @@ type ConfigField struct {
 type TargetView struct {
 	ID   string
 	Name string
+
+	// Deleted reports that this target's row is soft deleted.
+	// Only views built for historical display carry it set:
+	// every other projection is of a live row.
+	Deleted bool
+
 	Type   database.TargetType
 	Active bool
 	Config []ConfigField
 }
 
+// DisplayName is the name to render, marked when the target has
+// been deleted. Templates showing a name against historical data
+// must use it rather than Name, which stays the stored name.
+func (v TargetView) DisplayName() string {
+	if v.Deleted {
+		return v.Name + deletedNameSuffix
+	}
+
+	return v.Name
+}
+
 // NewTargetViews projects targets for rendering, replacing
 // each stored configuration blob with named, display-safe
 // fields.
+//
+// A soft-deleted row projects exactly as a live one does, minus
+// the deleted marker on its name: masking is a property of the
+// projection, not of the row's state, so a deleted target's
+// credential is as unreachable from a template as a live
+// target's.
 func NewTargetViews(
 	targets []database.Target,
 ) []TargetView {
@@ -49,6 +79,7 @@ func NewTargetViews(
 		views = append(views, TargetView{
 			ID:      t.ID,
 			Name:    t.Name,
+			Deleted: t.DeletedAt.Valid,
 			Type:    t.Type,
 			Active:  t.Active,
 			Config:  targetConfigFields(t),

internal/delivery/target_config_view_test.go

@@ -2,9 +2,11 @@ package delivery_test
 
 import (
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
 )
@@ -17,6 +19,14 @@ const (
 	slackWebhookURL = "https://hooks.slack.com" +
 		slackSecretPath
 
+	// slackMaskedURL is what a Slack webhook URL renders as
+	// once masked: scheme and host, path elided.
+	slackMaskedURL = "https://hooks.slack.com/..."
+
+	// slackTargetName is the target name the Slack projection
+	// tests use.
+	slackTargetName = "slack-target"
+
 	viewExampleOrigin = "https://example.com"
 	viewExampleHook   = viewExampleOrigin + "/hook"
 	viewMaskedOrigin  = viewExampleOrigin + "/..."
@@ -33,7 +43,7 @@ func TestMaskedWebhookURL(t *testing.T) {
 	}{
 		"slack webhook": {
 			url:  slackWebhookURL,
-			want: "https://hooks.slack.com/...",
+			want: slackMaskedURL,
 		},
 		"query string dropped": {
 			url:  viewExampleOrigin + "/a?token=secret",
@@ -125,23 +135,61 @@ func viewFor(
 	return views[0]
 }
 
-func TestNewTargetViews_Slack(t *testing.T) {
+// TestNewTargetViews_DeletedTarget proves the projection marks
+// a soft-deleted target's name and masks its configuration by
+// the same rules a live target's is. Delivery history outlives
+// the target it names, so this projection is what an operator
+// reads about a target that no longer exists.
+func TestNewTargetViews_DeletedTarget(t *testing.T) {
 	t.Parallel()
 
-	view := viewFor(t, database.Target{
+	target := slackTarget()
-		Name:   "slack-target",
+	target.DeletedAt = gorm.DeletedAt{
+		Time:  time.Now(),
+		Valid: true,
+	}
+
+	view := viewFor(t, target)
+
+	assert.True(t, view.Deleted)
+	assert.Equal(t, slackTargetName, view.Name)
+	assert.Equal(
+		t, slackTargetName+" (deleted)", view.DisplayName(),
+	)
+	assert.Equal(
+		t,
+		map[string]string{"Webhook URL": slackMaskedURL},
+		fieldMap(view.Config),
+	)
+}
+
+// slackTarget is the live Slack target the projection tests
+// share.
+func slackTarget() database.Target {
+	return database.Target{
+		Name:   slackTargetName,
 		Type:   database.TargetTypeSlack,
 		Active: true,
 		Config: `{"webhookUrl":"` +
 			slackWebhookURL + `"}`,
-	})
+	}
+}
+
+func TestNewTargetViews_Slack(t *testing.T) {
+	t.Parallel()
+
+	view := viewFor(t, slackTarget())
+
+	assert.Equal(t, slackTargetName, view.Name)
+
+	// A live target is never marked, so the marker cannot
+	// reach a name that still exists.
+	assert.False(t, view.Deleted)
+	assert.Equal(t, slackTargetName, view.DisplayName())
 
-	assert.Equal(t, "slack-target", view.Name)
 	assert.Equal(
 		t,
-		map[string]string{
+		map[string]string{"Webhook URL": slackMaskedURL},
-			"Webhook URL": "https://hooks.slack.com/...",
-		},
 		fieldMap(view.Config),
 	)
 }
@@ -212,7 +260,7 @@ func TestNewTargetViews_HTTPMasksDestinationURL(t *testing.T) {
 
 	assert.Equal(
 		t,
-		"https://hooks.slack.com/...",
+		slackMaskedURL,
 		fields["Destination URL"],
 	)
 

internal/handlers/source_logs_deleted_target_test.go

@@ -0,0 +1,144 @@
+package handlers_test
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/database"
+	"sneak.berlin/go/webhooker/internal/handlers"
+	"sneak.berlin/go/webhooker/internal/session"
+)
+
+// deletedMarker is the suffix the event log appends to the name
+// of a target that no longer exists.
+const deletedMarker = " (deleted)"
+
+// deleteTargetThroughHandler removes a target through the real
+// deletion handler, so the test soft-deletes exactly the way the
+// UI does rather than by writing the timestamp itself.
+func deleteTargetThroughHandler(
+	t *testing.T,
+	h *handlers.Handlers,
+	sess *session.Session,
+	webhookID, targetID string,
+) {
+	t.Helper()
+
+	req := postRequest(
+		"/source/"+webhookID+"/targets/"+targetID+"/delete",
+		authenticatedCookies(
+			t, sess, deleteTestUserID, deleteTestUsername,
+		),
+		map[string]string{
+			paramSourceID: webhookID,
+			paramTargetID: targetID,
+		},
+	)
+	w := httptest.NewRecorder()
+
+	h.HandleTargetDelete().ServeHTTP(w, req)
+
+	require.Equal(t, http.StatusSeeOther, w.Code)
+}
+
+// TestHandleSourceLogs_NamesDeletedTarget proves a delivery
+// produced by a since-deleted target still names it on the event
+// log, marked as deleted.
+//
+// Deletes are soft and deliveries carry no foreign key to the
+// target row, so the history outlives the target. Against a
+// scoped lookup the delivery resolves to a zero view and the page
+// renders ": delivered" with nothing saying what it was delivered
+// to.
+func TestHandleSourceLogs_NamesDeletedTarget(t *testing.T) {
+	t.Parallel()
+
+	var (
+		h     *handlers.Handlers
+		sess  *session.Session
+		db    *database.Database
+		dbMgr *database.WebhookDBManager
+	)
+
+	app := newTestApp(t, &h, &sess, &db, &dbMgr)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	wh := seedWebhook(t, db)
+	tgt := seedTarget(t, db, wh.ID, database.TargetTypeLog)
+
+	seedDeliveredEvent(t, dbMgr, wh.ID, tgt.ID)
+
+	// The control: the name is on the page while the target
+	// lives, and is not yet marked as deleted.
+	before := renderSourceLogsPage(t, h, sess, wh.ID)
+	assert.Contains(t, before, tgt.Name)
+	assert.NotContains(t, before, tgt.Name+deletedMarker)
+
+	deleteTargetThroughHandler(t, h, sess, wh.ID, tgt.ID)
+
+	after := renderSourceLogsPage(t, h, sess, wh.ID)
+
+	assert.Contains(
+		t, after, tgt.Name+deletedMarker,
+		"a delivery from a deleted target must keep its name, "+
+			"marked as no longer existing",
+	)
+	assert.Contains(
+		t, after, "delivered",
+		"the delivery history itself must survive the delete",
+	)
+}
+
+// TestHandleSourceLogs_MasksDeletedTargetConfig proves that
+// naming a deleted target does not widen what the page shows of
+// it: its stored configuration stays masked by exactly the rules
+// a live target's is.
+//
+// The lookup behind the name reads soft-deleted rows, so it
+// carries a full target row — credential blob included — into the
+// place a zero value used to sit. The projection to TargetView is
+// what keeps that blob away from the template, and it must hold
+// for a deleted row too.
+func TestHandleSourceLogs_MasksDeletedTargetConfig(t *testing.T) {
+	t.Parallel()
+
+	var (
+		h     *handlers.Handlers
+		sess  *session.Session
+		db    *database.Database
+		dbMgr *database.WebhookDBManager
+	)
+
+	app := newTestApp(t, &h, &sess, &db, &dbMgr)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	wh := seedWebhook(t, db)
+	tgt := seedConfiguredTarget(
+		t, db, wh.ID,
+		database.TargetTypeSlack,
+		`{"webhookUrl":"`+slackWebhookURL+`"}`,
+	)
+
+	seedDeliveredEvent(t, dbMgr, wh.ID, tgt.ID)
+	deleteTargetThroughHandler(t, h, sess, wh.ID, tgt.ID)
+
+	body := renderSourceLogsPage(t, h, sess, wh.ID)
+
+	assert.NotContains(t, body, slackSecretPath)
+	assert.NotContains(t, body, "T00000000")
+	assert.NotContains(t, body, "B00000000")
+	assert.NotContains(
+		t, body, "XXXXXXXXXXXXXXXXXXXXXXXX",
+	)
+	assert.NotContains(t, body, "webhookUrl")
+
+	// The name is there; only the credential is not.
+	assert.Contains(t, body, tgt.Name+deletedMarker)
+}

internal/handlers/source_management.go

@@ -860,11 +860,16 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc {
 //
 // The load is Unscoped because deleting a target only soft
 // deletes the row while its deliveries survive in the
-// per-webhook database: a scoped load leaves those deliveries
+// per-webhook database. Both halves of the map need those rows:
-// with a zero redactor, which renders their response bodies
+// a scoped load leaves an old delivery with a zero redactor,
-// unredacted. Only the redactor half of the map is built from
+// which renders its response bodies unredacted, and with a zero
-// deleted rows. The view half, which is what the page lists,
+// view, which renders its target as a blank name.
-// stays scoped.
+//
+// This map is historical display only. It is built for the event
+// log page and reaches nothing but DeliveryView.Target: the
+// target list on the source detail page, the edit form and the
+// replay path each resolve targets themselves, and a deleted row
+// is refused there as before.
 func (h *Handlers) loadTargetMap(
 	webhookID string,
 ) (map[string]eventLogTarget, error) {
@@ -880,21 +885,18 @@ func (h *Handlers) loadTargetMap(
 	targetMap := make(
 		map[string]eventLogTarget, len(targets),
 	)
-	live := make([]database.Target, 0, len(targets))
 
 	for i := range targets {
 		targetMap[targets[i].ID] = eventLogTarget{
 			Redactor: delivery.NewRedactor(&targets[i]),
 		}
-
-		if !targets[i].DeletedAt.Valid {
-			live = append(live, targets[i])
-		}
 	}
 
 	// The views come from NewTargetViews rather than being
-	// rebuilt here, so the masking rules stay in one place.
+	// rebuilt here, so the masking rules stay in one place and a
-	for _, v := range delivery.NewTargetViews(live) {
+	// deleted target's configuration is masked by the same code
+	// that masks a live one's.
+	for _, v := range delivery.NewTargetViews(targets) {
 		entry := targetMap[v.ID]
 		entry.View = v
 		targetMap[v.ID] = entry

templates/source_logs.html

@@ -39,7 +39,7 @@
                     <div class="flex items-center gap-4">
                         {{range .Deliveries}}
                         <span class="text-xs {{if eq .Status "delivered"}}text-green-600{{else if eq .Status "failed"}}text-red-600{{else if eq .Status "retrying"}}text-yellow-600{{else}}text-gray-400{{end}}">
-                            {{.Target.Name}}: {{.Status}}
+                            {{.Target.DisplayName}}: {{.Status}}
                         </span>
                         {{end}}
                         <span class="text-xs text-gray-400">{{.CreatedAt.Format "2006-01-02 15:04:05"}}</span>
@@ -74,7 +74,7 @@
                             <div class="py-2" x-data="{ attempts: false }">
                                 <div class="flex items-center justify-between cursor-pointer" @click="attempts = !attempts">
                                     <div class="flex items-center gap-3">
-                                        <span class="text-sm text-gray-700">{{.Target.Name}}</span>
+                                        <span class="text-sm text-gray-700">{{.Target.DisplayName}}</span>
                                         <span class="text-xs {{if eq .Status "delivered"}}text-green-600{{else if eq .Status "failed"}}text-red-600{{else if eq .Status "retrying"}}text-yellow-600{{else}}text-gray-400{{end}}">{{.Status}}</span>
                                     </div>
                                     <div class="flex items-center gap-3">