diff --git a/internal/delivery/target_config_view.go b/internal/delivery/target_config_view.go index 4174318..fc09a40 100644 --- a/internal/delivery/target_config_view.go +++ b/internal/delivery/target_config_view.go @@ -23,21 +23,51 @@ 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 // blob. type TargetView struct { - ID string - Name string + 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 { @@ -47,11 +77,12 @@ func NewTargetViews( t := &targets[i] views = append(views, TargetView{ - ID: t.ID, - Name: t.Name, - Type: t.Type, - Active: t.Active, - Config: targetConfigFields(t), + ID: t.ID, + Name: t.Name, + Deleted: t.DeletedAt.Valid, + Type: t.Type, + Active: t.Active, + Config: targetConfigFields(t), }) } diff --git a/internal/delivery/target_config_view_test.go b/internal/delivery/target_config_view_test.go index c634d73..41211fa 100644 --- a/internal/delivery/target_config_view_test.go +++ b/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{ - Name: "slack-target", + target := slackTarget() + 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{ - "Webhook URL": "https://hooks.slack.com/...", - }, + map[string]string{"Webhook URL": slackMaskedURL}, fieldMap(view.Config), ) } @@ -212,7 +260,7 @@ func TestNewTargetViews_HTTPMasksDestinationURL(t *testing.T) { assert.Equal( t, - "https://hooks.slack.com/...", + slackMaskedURL, fields["Destination URL"], ) diff --git a/internal/handlers/source_logs_deleted_target_test.go b/internal/handlers/source_logs_deleted_target_test.go new file mode 100644 index 0000000..31c7a1f --- /dev/null +++ b/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) +} diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index b83e843..e375f97 100644 --- a/internal/handlers/source_management.go +++ b/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 -// with a zero redactor, which renders their response bodies -// unredacted. Only the redactor half of the map is built from -// deleted rows. The view half, which is what the page lists, -// stays scoped. +// per-webhook database. Both halves of the map need those rows: +// a scoped load leaves an old delivery with a zero redactor, +// which renders its response bodies unredacted, and with a zero +// view, which renders its target as a blank name. +// +// 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. - for _, v := range delivery.NewTargetViews(live) { + // rebuilt here, so the masking rules stay in one place and a + // 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 diff --git a/templates/source_logs.html b/templates/source_logs.html index 0dc1981..110bd43 100644 --- a/templates/source_logs.html +++ b/templates/source_logs.html @@ -39,7 +39,7 @@
{{range .Deliveries}} - {{.Target.Name}}: {{.Status}} + {{.Target.DisplayName}}: {{.Status}} {{end}} {{.CreatedAt.Format "2006-01-02 15:04:05"}} @@ -74,7 +74,7 @@
- {{.Target.Name}} + {{.Target.DisplayName}} {{.Status}}