diff --git a/internal/handlers/auth.go b/internal/handlers/auth.go index 6f037af..74ad68d 100644 --- a/internal/handlers/auth.go +++ b/internal/handlers/auth.go @@ -333,7 +333,9 @@ func (h *Handlers) HandleLogout() http.HandlerFunc { ) } - // Redirect to login page - http.Redirect(w, r, "/pages/login", http.StatusSeeOther) + http.Redirect( + w, r, withNotice("/pages/login", signedOut), + http.StatusSeeOther, + ) } } diff --git a/internal/handlers/delivery_replay.go b/internal/handlers/delivery_replay.go index ced78b8..4ef278b 100644 --- a/internal/handlers/delivery_replay.go +++ b/internal/handlers/delivery_replay.go @@ -11,72 +11,37 @@ import ( "sneak.berlin/go/webhooker/internal/delivery" ) -// replayOutcomeParam is the query parameter the replay POST redirects -// with and the event log page reads its banner from. -const replayOutcomeParam = "replay" - -// replayOutcomeCode is the outcome of a replay POST. The redirect -// carries one of these fixed codes rather than a message, so nothing a -// client submits can reach the rendered page through it. -type replayOutcomeCode string - +// The outcomes of a replay POST, as the notice codes its redirect +// carries. noticeFor holds the line each one shows. const ( // replayQueued reports that a new delivery was created and handed // to the delivery engine. - replayQueued replayOutcomeCode = "queued" + replayQueued noticeCode = "replay-queued" // replayTargetDeleted reports a target that once existed and has // since been deleted. Deletes are soft and deliveries carry no // foreign key to the target row, so the history survives its // target and this is the ordinary case for an old event. - replayTargetDeleted replayOutcomeCode = "target-deleted" + replayTargetDeleted noticeCode = "replay-target-deleted" // replayTargetMissing reports a target id that names no row at // all, deleted or otherwise. - replayTargetMissing replayOutcomeCode = "target-missing" + replayTargetMissing noticeCode = "replay-target-missing" // replayTargetInactive reports a target the operator has // deactivated. A deactivated target receives no new deliveries, so // a replay to it would be a delivery they switched off. - replayTargetInactive replayOutcomeCode = "target-inactive" + replayTargetInactive noticeCode = "replay-target-inactive" // replayNotTerminal reports a delivery the engine has not finished // with. - replayNotTerminal replayOutcomeCode = "not-terminal" + replayNotTerminal noticeCode = "replay-not-terminal" // replayInFlight reports that an earlier replay of this event to // this target is still running. - replayInFlight replayOutcomeCode = "in-flight" + replayInFlight noticeCode = "replay-in-flight" ) -// replayOutcome returns the banner the event log page shows for an -// outcome code, and whether the replay was queued. An unrecognised -// code yields no banner. -func replayOutcome(code string) (string, bool) { - switch replayOutcomeCode(code) { - case replayQueued: - return "Replay queued: a new delivery was created against " + - "the target's current configuration.", true - case replayTargetDeleted: - return "Not replayed: the target this delivery was for has " + - "been deleted. Recreate the target, then replay.", false - case replayTargetMissing: - return "Not replayed: the target this delivery was for no " + - "longer exists.", false - case replayTargetInactive: - return "Not replayed: the target this delivery was for is " + - "deactivated. Activate it, then replay.", false - case replayNotTerminal: - return "Not replayed: this delivery has not finished yet.", - false - case replayInFlight: - return "Not replayed: a delivery of this event to this " + - "target is already in flight.", false - default: - return "", false - } -} - // HandleDeliveryReplay re-sends a finished delivery's event to its // target. // @@ -140,14 +105,14 @@ func (h *Handlers) replayDelivery( } if !original.Status.Terminal() { - h.finishReplay(w, r, webhook, replayNotTerminal) + redirectToEventLog(w, r, webhook, replayNotTerminal) return } target, code := h.replayTarget(webhook.ID, original.TargetID) if target == nil { - h.finishReplay(w, r, webhook, code) + redirectToEventLog(w, r, webhook, code) return } @@ -200,7 +165,7 @@ func (h *Handlers) queueReplay( } if inFlight > 0 { - h.finishReplay(w, r, webhook, replayInFlight) + redirectToEventLog(w, r, webhook, replayInFlight) return } @@ -238,7 +203,7 @@ func (h *Handlers) queueReplay( "delivery_id", task.DeliveryID, ) - h.finishReplay(w, r, webhook, replayQueued) + redirectToEventLog(w, r, webhook, replayQueued) } // replayTarget loads the delivery's target as it stands now. @@ -251,7 +216,7 @@ func (h *Handlers) queueReplay( // with the returned code saying why. func (h *Handlers) replayTarget( webhookID, targetID string, -) (*database.Target, replayOutcomeCode) { +) (*database.Target, noticeCode) { var target database.Target err := h.db.DB().Unscoped().Where( @@ -361,17 +326,16 @@ func replayBody(body string) *string { return &body } -// finishReplay redirects back to the event log the replay was -// triggered from, carrying the outcome code the page turns into a -// banner and the page number the form submitted. -func (h *Handlers) finishReplay( +// redirectToEventLog redirects a replay or resubmit back to the event +// log it was triggered from, carrying the outcome as its notice and +// the page number the form submitted. +func redirectToEventLog( w http.ResponseWriter, r *http.Request, webhook database.Webhook, - code replayOutcomeCode, + code noticeCode, ) { - dest := "/hook/" + webhook.ID + "/events?" + - replayOutcomeParam + "=" + string(code) + dest := withNotice("/hook/"+webhook.ID+"/events", code) // The page is read from the form rather than the query string: // this is a POST, and its query string is what logs and Referer diff --git a/internal/handlers/delivery_replay_test.go b/internal/handlers/delivery_replay_test.go index a301237..f155c17 100644 --- a/internal/handlers/delivery_replay_test.go +++ b/internal/handlers/delivery_replay_test.go @@ -212,7 +212,7 @@ func TestHandleDeliveryReplay_AppendsDeliveryAndLeavesOriginal( require.Equal(t, http.StatusSeeOther, w.Code) assert.Equal( t, - "/hook/"+wh.ID+"/events?replay=queued", + "/hook/"+wh.ID+"/events?notice=replay-queued", w.Header().Get("Location"), ) @@ -362,7 +362,7 @@ func TestHandleDeliveryReplay_RefusesDeletedTarget(t *testing.T) { require.Equal(t, http.StatusSeeOther, w.Code) assert.Equal( t, - "/hook/"+wh.ID+"/events?replay=target-deleted", + "/hook/"+wh.ID+"/events?notice=replay-target-deleted", w.Header().Get("Location"), ) @@ -390,7 +390,7 @@ func TestHandleDeliveryReplay_RefusesDeletedTarget(t *testing.T) { require.Equal(t, http.StatusSeeOther, missing.Code) assert.Equal( t, - "/hook/"+wh.ID+"/events?replay=target-missing", + "/hook/"+wh.ID+"/events?notice=replay-target-missing", missing.Header().Get("Location"), ) } @@ -431,7 +431,7 @@ func TestHandleDeliveryReplay_RefusesWhileEarlierReplayInFlight( require.Equal(t, http.StatusSeeOther, first.Code) require.Equal( t, - "/hook/"+wh.ID+"/events?replay=queued", + "/hook/"+wh.ID+"/events?notice=replay-queued", first.Header().Get("Location"), ) @@ -439,7 +439,7 @@ func TestHandleDeliveryReplay_RefusesWhileEarlierReplayInFlight( require.Equal(t, http.StatusSeeOther, second.Code) assert.Equal( t, - "/hook/"+wh.ID+"/events?replay=in-flight", + "/hook/"+wh.ID+"/events?notice=replay-in-flight", second.Header().Get("Location"), ) @@ -465,7 +465,7 @@ func TestHandleDeliveryReplay_RefusesWhileEarlierReplayInFlight( require.Equal(t, http.StatusSeeOther, pending.Code) assert.Equal( t, - "/hook/"+wh.ID+"/events?replay=not-terminal", + "/hook/"+wh.ID+"/events?notice=replay-not-terminal", pending.Header().Get("Location"), ) } @@ -509,7 +509,7 @@ func TestHandleSourceLogs_RendersReplayControlAndBanner(t *testing.T) { assert.Contains(t, body, ">Replay<") refused := renderSourceLogsPageWithQuery( - t, h, sess, wh.ID, "?replay=target-deleted", + t, h, sess, wh.ID, "?notice=replay-target-deleted", ) assert.Contains(t, refused, "alert-error") @@ -517,7 +517,7 @@ func TestHandleSourceLogs_RendersReplayControlAndBanner(t *testing.T) { // An outcome code nobody issued renders no banner at all. unknown := renderSourceLogsPageWithQuery( - t, h, sess, wh.ID, "?replay=made-up", + t, h, sess, wh.ID, "?notice=made-up", ) assert.NotContains(t, unknown, "alert-error") diff --git a/internal/handlers/event_resubmit.go b/internal/handlers/event_resubmit.go index dcff688..63b51c4 100644 --- a/internal/handlers/event_resubmit.go +++ b/internal/handlers/event_resubmit.go @@ -3,7 +3,6 @@ package handlers import ( "errors" "net/http" - "strconv" "github.com/go-chi/chi" "github.com/google/uuid" @@ -11,43 +10,19 @@ import ( "sneak.berlin/go/webhooker/internal/database" ) -// resubmitOutcomeParam is the query parameter the resubmit POST -// redirects with and the event log page reads its banner from. -const resubmitOutcomeParam = "resubmit" - -// resubmitOutcomeCode is the outcome of a resubmit POST. The redirect -// carries one of these fixed codes rather than a message, so nothing a -// client submits can reach the rendered page through it. -type resubmitOutcomeCode string - +// The outcomes of a resubmit POST, as the notice codes its redirect +// carries. noticeFor holds the line each one shows. const ( // resubmitQueued reports that a new event was stored and its // deliveries handed to the delivery engine. - resubmitQueued resubmitOutcomeCode = "queued" + resubmitQueued noticeCode = "resubmit-queued" // resubmitNoTargets reports a source with no active targets. The // new event is stored either way, exactly as a received event // with no targets is. - resubmitNoTargets resubmitOutcomeCode = "no-targets" + resubmitNoTargets noticeCode = "resubmit-no-targets" ) -// resubmitOutcome returns the banner the event log page shows for an -// outcome code, and whether the resubmit was queued. An unrecognised -// code yields no banner. -func resubmitOutcome(code string) (string, bool) { - switch resubmitOutcomeCode(code) { - case resubmitQueued: - return "Resubmitted: a new event was created from the stored " + - "one and queued to every active target.", true - case resubmitNoTargets: - return "Resubmitted: a new event was created, but this " + - "source has no active targets, so nothing was queued.", - true - default: - return "", false - } -} - // resubmitSource is the stored event a resubmit copies. Its body is // read as bytes rather than as a string so the copy is byte-identical // to what was received, whatever the payload's encoding. @@ -245,29 +220,5 @@ func (h *Handlers) queueResubmit( code = resubmitNoTargets } - h.finishResubmit(w, r, webhook, code) -} - -// finishResubmit redirects back to the event log the resubmit was -// triggered from, carrying the outcome code the page turns into a -// banner and the page number the form submitted. -func (h *Handlers) finishResubmit( - w http.ResponseWriter, - r *http.Request, - webhook database.Webhook, - code resubmitOutcomeCode, -) { - dest := "/hook/" + webhook.ID + "/events?" + - resubmitOutcomeParam + "=" + string(code) - - // The page is read from the form rather than the query string: - // this is a POST, and its query string is what logs and Referer - // headers record. - if page := pageOrFirst( - r.PostFormValue("page"), - ); page > 1 { - dest += "&page=" + strconv.Itoa(page) - } - - http.Redirect(w, r, dest, http.StatusSeeOther) + redirectToEventLog(w, r, webhook, code) } diff --git a/internal/handlers/event_resubmit_test.go b/internal/handlers/event_resubmit_test.go index 921139d..a7c92d2 100644 --- a/internal/handlers/event_resubmit_test.go +++ b/internal/handlers/event_resubmit_test.go @@ -154,7 +154,7 @@ func TestHandleEventResubmit_DeliversToTargetCreatedAfterTheEvent( require.Equal(t, http.StatusSeeOther, w.Code) assert.Equal( t, - "/hook/"+wh.ID+"/events?resubmit=queued", + "/hook/"+wh.ID+"/events?notice=resubmit-queued", w.Header().Get("Location"), ) @@ -282,7 +282,7 @@ func TestHandleEventResubmit_IsRepeatable(t *testing.T) { require.Equal(t, http.StatusSeeOther, w.Code) assert.Equal( t, - "/hook/"+wh.ID+"/events?resubmit=queued", + "/hook/"+wh.ID+"/events?notice=resubmit-queued", w.Header().Get("Location"), "a resubmit must not be refused while an earlier "+ "one is in flight", @@ -436,7 +436,7 @@ func TestHandleEventResubmit_SkipsInactiveTarget(t *testing.T) { require.Equal(t, http.StatusSeeOther, w.Code) assert.Equal( t, - "/hook/"+wh.ID+"/events?resubmit=queued", + "/hook/"+wh.ID+"/events?notice=resubmit-queued", w.Header().Get("Location"), "an inactive target is skipped, not an error", ) @@ -482,7 +482,7 @@ func TestHandleEventResubmit_NoActiveTargetsStillStoresEvent( require.Equal(t, http.StatusSeeOther, w.Code) assert.Equal( t, - "/hook/"+wh.ID+"/events?resubmit=no-targets", + "/hook/"+wh.ID+"/events?notice=resubmit-no-targets", w.Header().Get("Location"), ) diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 6eca35f..9aa280c 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -94,10 +94,10 @@ type Handlers struct { // parsePageTemplate parses a page-specific template set from the // embedded FS. Each page template is combined with the shared -// base, htmlheader, and navbar templates, and with any further files -// the page includes. The page file must be listed first so that its -// root action ({{template "base" .}}) becomes the template set's entry -// point. +// base, htmlheader, navbar and notice templates, and with any further +// files the page includes. The page file must be listed first so that +// its root action ({{template "base" .}}) becomes the template set's +// entry point. func parsePageTemplate( pageFile string, included ...string, ) *template.Template { @@ -106,6 +106,7 @@ func parsePageTemplate( "base.html", "htmlheader.html", "navbar.html", + "notice.html", }, included...) return template.Must( @@ -206,11 +207,13 @@ func (s *Handlers) renderError( // served outside the routes where NoCache runs. w.Header().Set("Cache-Control", "no-store") + // No notice: one would say an action worked above a page saying + // the request failed. data := s.pageData(r, map[string]any{ "Status": status, "StatusText": http.StatusText(status), "Message": errorPageText(status), - }) + }, nil) var buf bytes.Buffer @@ -264,6 +267,7 @@ type templateDataWrapper struct { User *UserInfo CSRFToken string Version string + Notice *notice Data any } @@ -308,12 +312,15 @@ func (s *Handlers) renderTemplate( return } - s.executeTemplate(w, r, tmpl, s.pageData(r, data)) + s.executeTemplate(w, r, tmpl, s.pageData(r, data, noticeFor(r))) } // pageData adds the fields the shared layout renders to a page's own -// data. -func (s *Handlers) pageData(r *http.Request, data any) any { +// data. The layout shows the notice, when there is one, above the +// page. +func (s *Handlers) pageData( + r *http.Request, data any, pageNotice *notice, +) any { userInfo := s.getUserInfo(r) csrfToken := middleware.CSRFToken(r) @@ -327,6 +334,7 @@ func (s *Handlers) pageData(r *http.Request, data any) any { m["User"] = userInfo m["CSRFToken"] = csrfToken m["Version"] = version + m["Notice"] = pageNotice return m } @@ -335,6 +343,7 @@ func (s *Handlers) pageData(r *http.Request, data any) any { User: userInfo, CSRFToken: csrfToken, Version: version, + Notice: pageNotice, Data: data, } } diff --git a/internal/handlers/notice.go b/internal/handlers/notice.go new file mode 100644 index 0000000..fa924ab --- /dev/null +++ b/internal/handlers/notice.go @@ -0,0 +1,109 @@ +package handlers + +import "net/http" + +// noticeParam is the query parameter an action's redirect carries its +// notice code in. +const noticeParam = "notice" + +// noticeCode names one of the fixed lines noticeFor knows. An action +// redirects with the code rather than the line, so nothing a client +// puts in the URL reaches the page: a code noticeFor does not know +// shows nothing. +type noticeCode string + +// The codes of the actions on the webhook pages and of signing out. +// Replay's and resubmit's are beside those actions, with the reasons +// each can be refused. +const ( + webhookCreated noticeCode = "webhook-created" + webhookSaved noticeCode = "webhook-saved" + webhookDeleted noticeCode = "webhook-deleted" + entrypointAdded noticeCode = "entrypoint-added" + entrypointDeleted noticeCode = "entrypoint-deleted" + entrypointActivated noticeCode = "entrypoint-activated" + entrypointDeactivated noticeCode = "entrypoint-deactivated" + targetAdded noticeCode = "target-added" + targetSaved noticeCode = "target-saved" + targetDeleted noticeCode = "target-deleted" + targetActivated noticeCode = "target-activated" + targetDeactivated noticeCode = "target-deactivated" + signedOut noticeCode = "signed-out" +) + +// notice is the line templates/notice.html shows above a page to say +// what an action did. +type notice struct { + Text string + + // Failed shows the line as an error: the action was refused. + Failed bool +} + +// noticeFor returns the notice the request's URL names, or nil when it +// names none or an unknown code. +func noticeFor(r *http.Request) *notice { + n, ok := map[noticeCode]notice{ + webhookCreated: {Text: "Webhook created."}, + webhookSaved: {Text: "Webhook saved."}, + webhookDeleted: {Text: "Webhook deleted."}, + entrypointAdded: {Text: "Entrypoint added."}, + entrypointDeleted: {Text: "Entrypoint deleted."}, + entrypointActivated: {Text: "Entrypoint activated."}, + entrypointDeactivated: {Text: "Entrypoint deactivated."}, + targetAdded: {Text: "Target added."}, + targetSaved: {Text: "Target saved."}, + targetDeleted: {Text: "Target deleted."}, + targetActivated: {Text: "Target activated."}, + targetDeactivated: {Text: "Target deactivated."}, + signedOut: {Text: "Signed out."}, + + replayQueued: { + Text: "Replay queued: a new delivery was created " + + "against the target's current configuration.", + }, + replayTargetDeleted: { + Text: "Not replayed: the target this delivery was for " + + "has been deleted. Recreate the target, then replay.", + Failed: true, + }, + replayTargetMissing: { + Text: "Not replayed: the target this delivery was for " + + "no longer exists.", + Failed: true, + }, + replayTargetInactive: { + Text: "Not replayed: the target this delivery was for " + + "is deactivated. Activate it, then replay.", + Failed: true, + }, + replayNotTerminal: { + Text: "Not replayed: this delivery has not finished yet.", + Failed: true, + }, + replayInFlight: { + Text: "Not replayed: a delivery of this event to this " + + "target is already in flight.", + Failed: true, + }, + + resubmitQueued: { + Text: "Resubmitted: a new event was created from the " + + "stored one and queued to every active target.", + }, + resubmitNoTargets: { + Text: "Resubmitted: a new event was created, but this " + + "source has no active targets, so nothing was queued.", + }, + }[noticeCode(r.URL.Query().Get(noticeParam))] + if !ok { + return nil + } + + return &n +} + +// withNotice returns path with code added as its notice. +func withNotice(path string, code noticeCode) string { + return path + "?" + noticeParam + "=" + string(code) +} diff --git a/internal/handlers/source_delete_test.go b/internal/handlers/source_delete_test.go index 58db5c5..1bdd9fc 100644 --- a/internal/handlers/source_delete_test.go +++ b/internal/handlers/source_delete_test.go @@ -411,7 +411,9 @@ func TestHandleSourceDelete_RemovesConfigAndEventDatabase( h.HandleSourceDelete().ServeHTTP(w, req) require.Equal(t, http.StatusSeeOther, w.Code) - assert.Equal(t, "/hooks", w.Header().Get("Location")) + assert.Equal( + t, "/hooks?notice=webhook-deleted", w.Header().Get("Location"), + ) assert.Equal( t, int64(0), diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index 330e787..1596101 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -322,7 +322,8 @@ func (h *Handlers) createWebhookWithEntrypoint( ) http.Redirect( - w, r, "/hook/"+webhook.ID, http.StatusSeeOther, + w, r, withNotice("/hook/"+webhook.ID, webhookCreated), + http.StatusSeeOther, ) } @@ -579,7 +580,8 @@ func (h *Handlers) applyWebhookEdit( } http.Redirect( - w, r, "/hook/"+webhook.ID, http.StatusSeeOther, + w, r, withNotice("/hook/"+webhook.ID, webhookSaved), + http.StatusSeeOther, ) } @@ -662,7 +664,9 @@ func (h *Handlers) deleteWebhookResources( return } - http.Redirect(w, r, "/hooks", http.StatusSeeOther) + http.Redirect( + w, r, withNotice("/hooks", webhookDeleted), http.StatusSeeOther, + ) } // commitWebhookDeletion soft-deletes a webhook's entrypoints, @@ -841,31 +845,16 @@ func (h *Handlers) HandleSourceLogs() http.HandlerFunc { totalPages++ } - // The banner a replay or resubmit POST redirected back - // with. The message comes from a fixed set keyed by the - // outcome code, never from the query string itself. - replayMsg, replayOK := replayOutcome( - r.URL.Query().Get(replayOutcomeParam), - ) - - resubmitMsg, resubmitOK := resubmitOutcome( - r.URL.Query().Get(resubmitOutcomeParam), - ) - data := map[string]any{ - tmplKeyWebhook: &webhook, - "Events": evts, - "ReplayMessage": replayMsg, - "ReplayQueued": replayOK, - "ResubmitMessage": resubmitMsg, - "ResubmitQueued": resubmitOK, - "Page": page, - "TotalPages": totalPages, - "TotalEvents": total, - "HasPrev": page > 1, - "HasNext": page < totalPages, - "PrevPage": page - 1, - "NextPage": page + 1, + tmplKeyWebhook: &webhook, + "Events": evts, + "Page": page, + "TotalPages": totalPages, + "TotalEvents": total, + "HasPrev": page > 1, + "HasNext": page < totalPages, + "PrevPage": page - 1, + "NextPage": page + 1, } h.renderTemplate(w, r, "source_logs.html", data) @@ -1254,7 +1243,8 @@ func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc { } http.Redirect( - w, r, "/hook/"+webhook.ID, http.StatusSeeOther, + w, r, withNotice("/hook/"+webhook.ID, entrypointAdded), + http.StatusSeeOther, ) } } @@ -1365,7 +1355,8 @@ func (h *Handlers) processTargetCreate( } http.Redirect( - w, r, "/hook/"+webhook.ID, http.StatusSeeOther, + w, r, withNotice("/hook/"+webhook.ID, targetAdded), + http.StatusSeeOther, ) } @@ -1643,6 +1634,7 @@ func (h *Handlers) HandleEntrypointDelete() http.HandlerFunc { "entrypointID", &database.Entrypoint{}, "failed to delete entrypoint", nil, + entrypointDeleted, ) } @@ -1655,18 +1647,21 @@ func (h *Handlers) HandleTargetDelete() http.HandlerFunc { "targetID", &database.Target{}, "failed to delete target", h.evictArchiveWriterIfUnused, + targetDeleted, ) } // deleteChildResource returns a handler that deletes a child // resource (entrypoint or target) belonging to a webhook. The // optional afterDelete hook runs with the webhook's id once the -// delete has succeeded, before the redirect. +// delete has succeeded, before the redirect, which carries done as +// its notice. func (h *Handlers) deleteChildResource( idParam string, model any, errMsg string, afterDelete func(webhookID string), + done noticeCode, ) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { userID, ok := h.getUserID(r) @@ -1708,7 +1703,7 @@ func (h *Handlers) deleteChildResource( http.Redirect( w, r, - "/hook/"+webhook.ID, + withNotice("/hook/"+webhook.ID, done), http.StatusSeeOther, ) } @@ -1719,7 +1714,7 @@ func (h *Handlers) deleteChildResource( func (h *Handlers) HandleEntrypointToggle() http.HandlerFunc { return h.toggleChildResource( "entrypointID", - func(webhookID, childID string) error { + func(webhookID, childID string) (bool, error) { var ep database.Entrypoint err := h.db.DB().Where( @@ -1727,14 +1722,15 @@ func (h *Handlers) HandleEntrypointToggle() http.HandlerFunc { childID, webhookID, ).First(&ep).Error if err != nil { - return err + return false, err } ep.Active = !ep.Active - return h.db.DB().Save(&ep).Error + return ep.Active, h.db.DB().Save(&ep).Error }, "failed to toggle entrypoint", + entrypointActivated, entrypointDeactivated, ) } @@ -1742,7 +1738,7 @@ func (h *Handlers) HandleEntrypointToggle() http.HandlerFunc { func (h *Handlers) HandleTargetToggle() http.HandlerFunc { return h.toggleChildResource( "targetID", - func(webhookID, childID string) error { + func(webhookID, childID string) (bool, error) { var tgt database.Target err := h.db.DB().Where( @@ -1750,23 +1746,27 @@ func (h *Handlers) HandleTargetToggle() http.HandlerFunc { childID, webhookID, ).First(&tgt).Error if err != nil { - return err + return false, err } tgt.Active = !tgt.Active - return h.db.DB().Save(&tgt).Error + return tgt.Active, h.db.DB().Save(&tgt).Error }, "failed to toggle target", + targetActivated, targetDeactivated, ) } // toggleChildResource returns a handler that toggles the active -// state of a child resource belonging to a webhook. +// state of a child resource belonging to a webhook. toggleFn returns +// the new state, and the redirect carries activated or deactivated as +// its notice to match. func (h *Handlers) toggleChildResource( idParam string, - toggleFn func(webhookID, childID string) error, + toggleFn func(webhookID, childID string) (bool, error), errMsg string, + activated, deactivated noticeCode, ) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { userID, ok := h.getUserID(r) @@ -1792,16 +1792,21 @@ func (h *Handlers) toggleChildResource( return } - err = toggleFn(webhook.ID, childID) + active, err := toggleFn(webhook.ID, childID) if err != nil { h.serverError(w, r, errMsg, err) return } + done := deactivated + if active { + done = activated + } + http.Redirect( w, r, - "/hook/"+webhook.ID, + withNotice("/hook/"+webhook.ID, done), http.StatusSeeOther, ) } diff --git a/internal/handlers/target_edit.go b/internal/handlers/target_edit.go index 810b801..d3264e3 100644 --- a/internal/handlers/target_edit.go +++ b/internal/handlers/target_edit.go @@ -161,7 +161,8 @@ func (h *Handlers) applyTargetEdit( } http.Redirect( - w, r, "/hook/"+webhook.ID, http.StatusSeeOther, + w, r, withNotice("/hook/"+webhook.ID, targetSaved), + http.StatusSeeOther, ) } diff --git a/internal/server/error_page_test.go b/internal/server/error_page_test.go index b907de8..06abf2e 100644 --- a/internal/server/error_page_test.go +++ b/internal/server/error_page_test.go @@ -83,6 +83,22 @@ func TestErrorPage_DeletedTarget(t *testing.T) { assertErrorPage(t, w, http.StatusNotFound, backToWebhooks) } +// TestErrorPage_ShowsNoNotice pins that a notice code in the URL of a +// page that fails is not shown above the error. +func TestErrorPage_ShowsNoNotice(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + + userID, _ := env.seedUser(t, "owner", "somepassword") + cookies := env.authCookies(t, userID, "owner") + + w := env.get("/hook/no-such-webhook?notice=webhook-saved", cookies) + + assertErrorPage(t, w, http.StatusNotFound, backToWebhooks) + assert.NotContains(t, w.Body.String(), "Webhook saved.") +} + func TestErrorPage_UnknownPath(t *testing.T) { t.Parallel() diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go index cb344e7..ad772a6 100644 --- a/internal/server/routes_test.go +++ b/internal/server/routes_test.go @@ -260,6 +260,24 @@ func (e *testEnv) urlFrom( return html.UnescapeString(match[1]) } +// requireNotice requires w to redirect to dest carrying the notice +// code, then renders that page and requires it to show text. +func (e *testEnv) requireNotice( + t *testing.T, + w *httptest.ResponseRecorder, + dest, code, text string, + cookies []*http.Cookie, +) { + t.Helper() + + require.Equal(t, http.StatusSeeOther, w.Code) + require.Equal(t, dest+"?notice="+code, w.Header().Get("Location")) + + page := e.get(w.Header().Get("Location"), cookies) + require.Equal(t, http.StatusOK, page.Code) + assert.Contains(t, page.Body.String(), text) +} + // authCookies forges an authenticated session for the given user. func (e *testEnv) authCookies( t *testing.T, @@ -738,6 +756,31 @@ func TestPagesLogin_ReturnsToTheRequestedPage(t *testing.T) { assert.Equal(t, asked, w.Header().Get("Location")) } +// TestPagesLogout_SaysSignedOut signs out with the navbar's form and +// lands on the sign-in page, which says so. +func TestPagesLogout_SaysSignedOut(t *testing.T) { + t.Parallel() + + env := newTestEnv(t) + + userID, _ := env.seedUser(t, "leaver", "somepassword") + token, cookies := env.csrfFrom( + t, "/hooks", env.authCookies(t, userID, "leaver"), + ) + + form := url.Values{} + form.Set("csrf_token", token) + + w := env.post( + env.urlFrom(t, "/hooks", `action="(/pages/logout)"`, cookies), + form, cookies, + ) + + // The sign-in page is requested without the session cookie, which + // the logout told the browser to delete. + env.requireNotice(t, w, "/pages/login", "signed-out", "Signed out.", nil) +} + // --- /user/{username} group --- // TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged @@ -855,9 +898,9 @@ func TestHooks_ListAndNewWebhookForm(t *testing.T) { require.NoError(t, env.db.DB().Where("name = ?", "created").First(&created).Error, ) - assert.Equal( - t, "/hook/"+created.ID, w.Header().Get("Location"), - "creating a webhook should redirect to its page", + env.requireNotice( + t, w, "/hook/"+created.ID, "webhook-created", "Webhook created.", + cookies, ) } @@ -888,8 +931,7 @@ func TestHook_EditFormAndDelete(t *testing.T) { env.urlFrom(t, editPage, `action="(/hook/[^/"]+/edit)"`, cookies), form, cookies, ) - require.Equal(t, http.StatusSeeOther, w.Code) - assert.Equal(t, page, w.Header().Get("Location")) + env.requireNotice(t, w, page, "webhook-saved", "Webhook saved.", cookies) var edited database.Webhook @@ -903,16 +945,17 @@ func TestHook_EditFormAndDelete(t *testing.T) { env.urlFrom(t, page, `action="(/hook/[^/"]+/delete)"`, cookies), form, cookies, ) - require.Equal(t, http.StatusSeeOther, w.Code) - assert.Equal(t, "/hooks", w.Header().Get("Location")) + env.requireNotice( + t, w, "/hooks", "webhook-deleted", "Webhook deleted.", cookies, + ) assert.Equal( t, http.StatusNotFound, env.get(page, cookies).Code, "a deleted webhook's page should be gone", ) } -// TestHook_EntrypointActions adds, deactivates and deletes an -// entrypoint with the forms on the webhook page, each submitted to +// TestHook_EntrypointActions adds, deactivates, activates and deletes +// an entrypoint with the forms on the webhook page, each submitted to // the action and with the token the page rendered. func TestHook_EntrypointActions(t *testing.T) { t.Parallel() @@ -930,16 +973,19 @@ func TestHook_EntrypointActions(t *testing.T) { form.Set("csrf_token", token) // submit posts the webhook page's form whose action pattern - // captures, and requires the redirect back to that page. - submit := func(pattern string) { + // captures, and requires the redirect back to that page with the + // notice code, and the page to show text. + submit := func(pattern, code, text string) { t.Helper() w := env.post(env.urlFrom(t, page, pattern, cookies), form, cookies) - require.Equal(t, http.StatusSeeOther, w.Code) - require.Equal(t, page, w.Header().Get("Location")) + env.requireNotice(t, w, page, code, text, cookies) } - submit(`action="(/hook/[^/"]+/entrypoints)"`) + toggle := `action="(/hook/[^/"]+/entrypoints/[^/"]+/toggle)"` + + submit(`action="(/hook/[^/"]+/entrypoints)"`, + "entrypoint-added", "Entrypoint added.") var added database.Entrypoint @@ -948,7 +994,7 @@ func TestHook_EntrypointActions(t *testing.T) { ) require.True(t, added.Active) - submit(`action="(/hook/[^/"]+/entrypoints/[^/"]+/toggle)"`) + submit(toggle, "entrypoint-deactivated", "Entrypoint deactivated.") var toggled database.Entrypoint @@ -957,7 +1003,10 @@ func TestHook_EntrypointActions(t *testing.T) { ) assert.False(t, toggled.Active, "the toggle should deactivate it") - submit(`action="(/hook/[^/"]+/entrypoints/[^/"]+/delete)"`) + submit(toggle, "entrypoint-activated", "Entrypoint activated.") + + submit(`action="(/hook/[^/"]+/entrypoints/[^/"]+/delete)"`, + "entrypoint-deleted", "Entrypoint deleted.") var left int64 @@ -968,8 +1017,8 @@ func TestHook_EntrypointActions(t *testing.T) { // TestHook_TargetActions adds a target with the form on the webhook // page, follows its Edit link to the target edit form and submits -// it, then deactivates and deletes it, every URL and token taken from -// the rendered pages. +// it, then deactivates, activates and deletes it, every URL and token +// taken from the rendered pages. func TestHook_TargetActions(t *testing.T) { t.Parallel() @@ -984,27 +1033,29 @@ func TestHook_TargetActions(t *testing.T) { // submit posts form, with the token, to the action pattern // captures on the page at from, and requires the redirect back to - // the webhook page. - submit := func(from, pattern string, form url.Values) { + // the webhook page with the notice code, and that page to show + // text. + submit := func(from, pattern string, form url.Values, code, text string) { t.Helper() form.Set("csrf_token", token) w := env.post(env.urlFrom(t, from, pattern, cookies), form, cookies) - require.Equal(t, http.StatusSeeOther, w.Code) - require.Equal(t, page, w.Header().Get("Location")) + env.requireNotice(t, w, page, code, text, cookies) } + toggle := `action="(/hook/[^/"]+/targets/[^/"]+/toggle)"` + submit(page, `action="(/hook/[^/"]+/targets)"`, url.Values{ "name": {"added"}, "type": {string(database.TargetTypeLog)}, - }) + }, "target-added", "Target added.") editPage := env.urlFrom( t, page, `href="(/hook/[^/"]+/targets/[^/"]+/edit)"`, cookies, ) submit(editPage, `action="(/hook/[^/"]+/targets/[^/"]+/edit)"`, - url.Values{"name": {"renamed"}}) + url.Values{"name": {"renamed"}}, "target-saved", "Target saved.") var edited database.Target @@ -1014,8 +1065,8 @@ func TestHook_TargetActions(t *testing.T) { assert.Equal(t, "renamed", edited.Name) require.True(t, edited.Active) - submit(page, `action="(/hook/[^/"]+/targets/[^/"]+/toggle)"`, - url.Values{}) + submit(page, toggle, url.Values{}, + "target-deactivated", "Target deactivated.") var toggled database.Target @@ -1024,8 +1075,11 @@ func TestHook_TargetActions(t *testing.T) { ) assert.False(t, toggled.Active, "the toggle should deactivate it") + submit(page, toggle, url.Values{}, + "target-activated", "Target activated.") + submit(page, `action="(/hook/[^/"]+/targets/[^/"]+/delete)"`, - url.Values{}) + url.Values{}, "target-deleted", "Target deleted.") var left int64 @@ -1061,9 +1115,9 @@ func TestHook_ResubmitFromEventLog(t *testing.T) { env.urlFrom(t, logsPath, `action="(/hook/[^"]+/resubmit)"`, cookies), form, cookies, ) - require.Equal(t, http.StatusSeeOther, w.Code) - assert.Equal( - t, logsPath+"?resubmit=no-targets", w.Header().Get("Location"), + env.requireNotice( + t, w, logsPath, "resubmit-no-targets", + "this source has no active targets", cookies, ) webhookDB, err := env.dbMgr.GetDB(wh.ID) @@ -1299,10 +1353,8 @@ func TestDeliveryReplay_PostOnlyAndCSRFProtected(t *testing.T) { html.UnescapeString(action[1]), form, cookies, ) - require.Equal(t, http.StatusSeeOther, w.Code) - assert.Equal( - t, logsPath+"?replay=queued", - w.Header().Get("Location"), + env.requireNotice( + t, w, logsPath, "replay-queued", "Replay queued:", cookies, ) assert.Equal( t, int64(2), env.countDeliveries(t, wh.ID), diff --git a/templates/base.html b/templates/base.html index b300710..a193d19 100644 --- a/templates/base.html +++ b/templates/base.html @@ -7,6 +7,7 @@