Add site title, HTML response template for errors

Handler now uses a generic HTML template to provide HTML responses for Bad Request and internal errors. It's written in such a way that we could easily support custom templates one day—but not today. Also extracted Handler.errorResponse from Handler.handleApiEvent, which makes the logic cleaner and testing easier.
mbland · Apr 3, 2023 · 08c372a · 08c372a
1 parent 5a83df0
commit 08c372a
Show file tree

Hide file tree

Showing 6 changed files with 206 additions and 16 deletions.
diff --git a/handler/handler.go b/handler/handler.go
@@ -7,6 +7,7 @@ import (
 	"log"
 	"net/http"
 	"strings"
+	"text/template"
 
 	"github.com/aws/aws-lambda-go/events"
 	"github.com/mbland/elistman/ops"
@@ -15,21 +16,32 @@ import (
 type RedirectMap map[ops.OperationResult]string
 
 type Handler struct {
-	UnsubscribeAddr string
-	Agent           ops.SubscriptionAgent
-	Redirects       RedirectMap
+	UnsubscribeAddr  string
+	SiteTitle        string
+	Agent            ops.SubscriptionAgent
+	Redirects        RedirectMap
+	responseTemplate *template.Template
 }
 
 func NewHandler(
 	emailDomain string,
+	siteTitle string,
 	agent ops.SubscriptionAgent,
 	paths RedirectPaths,
-) *Handler {
+	responseTemplate string,
+) (*Handler, error) {
 	fullUrl := func(path string) string {
 		return "https://" + emailDomain + "/" + path
 	}
+	responseTmpl, err := initResponseBodyTemplate(responseTemplate)
+
+	if err != nil {
+		return nil, err
+	}
+
 	return &Handler{
 		"unsubscribe@" + emailDomain,
+		siteTitle,
 		agent,
 		RedirectMap{
 			ops.Invalid:           fullUrl(paths.Invalid),
@@ -39,9 +51,44 @@ func NewHandler(
 			ops.NotSubscribed:     fullUrl(paths.NotSubscribed),
 			ops.Unsubscribed:      fullUrl(paths.Unsubscribed),
 		},
+		responseTmpl,
+	}, nil
+}
+
+func initResponseBodyTemplate(bodyTmpl string) (*template.Template, error) {
+	builder := &strings.Builder{}
+	params := &ResponseTemplateParams{}
+
+	if tmpl, err := template.New("responseBody").Parse(bodyTmpl); err != nil {
+		return nil, fmt.Errorf("parsing response body template failed: %s", err)
+	} else if err := tmpl.Execute(builder, params); err != nil {
+		return nil, fmt.Errorf(
+			"executing response body template failed: %s", err,
+		)
+	} else {
+		return tmpl, nil
 	}
 }
 
+const ResponseTemplate = `<!DOCTYPE html>
+<html lang="en-us">
+  <head>
+	<meta charset="UTF-8" />
+	<meta name="viewport" content="width=device-width, initial-scale=1.0" />
+	<title>{{.Title}} - {{.SiteTitle}}</title>
+  </head>
+  <body>
+    <h1>{{.Title}}</h1>
+    {{.Body}}
+  </body>
+</html>`
+
+type ResponseTemplateParams struct {
+	Title     string
+	SiteTitle string
+	Body      string
+}
+
 type errorWithStatus struct {
 	HttpStatus int
 	Message    string
@@ -80,16 +127,46 @@ func (h *Handler) handleApiEvent(
 	}
 
 	if err != nil {
-		errorStatus := http.StatusInternalServerError
-		if apiErr, ok := err.(*errorWithStatus); ok {
-			errorStatus = apiErr.HttpStatus
-		}
-		res = &events.APIGatewayV2HTTPResponse{StatusCode: errorStatus}
+		res = h.errorResponse(err)
 	}
 	logApiResponse(origReq, res, err)
 	return res
 }
 
+func (h *Handler) addResponseBody(
+	res *events.APIGatewayV2HTTPResponse, body string,
+) {
+	httpStatus := res.StatusCode
+	title := fmt.Sprintf("%d %s", httpStatus, http.StatusText(httpStatus))
+	params := &ResponseTemplateParams{title, h.SiteTitle, body}
+	builder := &strings.Builder{}
+
+	if err := h.responseTemplate.Execute(builder, params); err != nil {
+		// This should never happen, but if it does, fall back to plain text.
+		log.Printf("ERROR adding HTML response body: %s: %+v", err, params)
+		res.Headers["content-type"] = "text/plain; charset=utf-8"
+		res.Body = fmt.Sprintf("%s - %s\n\n%s\n", title, h.SiteTitle, body)
+	} else {
+		res.Headers["content-type"] = "text/html; charset=utf-8"
+		res.Body = builder.String()
+	}
+}
+
+func (h *Handler) errorResponse(err error) *events.APIGatewayV2HTTPResponse {
+	res := &events.APIGatewayV2HTTPResponse{
+		StatusCode: http.StatusInternalServerError,
+		Headers:    map[string]string{},
+	}
+	if apiErr, ok := err.(*errorWithStatus); ok {
+		res.StatusCode = apiErr.HttpStatus
+	}
+
+	body := "<p>There was a problem on our end; " +
+		"please try again in a few minutes.</p>\n"
+	h.addResponseBody(res, body)
+	return res
+}
+
 func logApiResponse(
 	req *events.APIGatewayV2HTTPRequest,
 	res *events.APIGatewayV2HTTPResponse,
@@ -188,7 +265,10 @@ func (h *Handler) respondToParseError(
 	// it's a bad machine generated request.
 	if !errors.Is(err, &ParseError{Type: SubscribeOp}) {
 		response.StatusCode = http.StatusBadRequest
-		response.Body = fmt.Sprintf("%s\n", err)
+		body := "<p>Parsing the request failed:</p>\n" +
+			"<pre>\n" + template.HTMLEscapeString(err.Error()) + "\n</pre>\n" +
+			"<p>Please correct the request and try again.</p>"
+		h.addResponseBody(response, body)
 	} else if redirect, ok := h.Redirects[ops.Invalid]; !ok {
 		return nil, errors.New("no redirect for invalid operation")
 	} else {

diff --git a/handler/handler_test.go b/handler/handler_test.go
@@ -8,6 +8,7 @@ import (
 	"net/http"
 	"strings"
 	"testing"
+	"text/template"
 
 	"github.com/aws/aws-lambda-go/events"
 	"github.com/google/uuid"
@@ -51,6 +52,7 @@ type fixture struct {
 }
 
 const testEmailDomain = "mike-bland.com"
+const testSiteTitle = "Mike Bland's blog"
 const testUnsubscribeAddress = "unsubscribe@" + testEmailDomain
 
 // const testValidUid = "00000000-1111-2222-3333-444444444444"
@@ -66,14 +68,50 @@ var testRedirects = RedirectPaths{
 
 func newFixture() *fixture {
 	ta := &testAgent{}
-	return &fixture{ta: ta, h: NewHandler(testEmailDomain, ta, testRedirects)}
+	handler, err := NewHandler(
+		testEmailDomain, testSiteTitle, ta, testRedirects, ResponseTemplate,
+	)
+
+	if err != nil {
+		panic(err.Error())
+	}
+	return &fixture{ta: ta, h: handler}
+}
+
+func TestInitResponseBodyTemplate(t *testing.T) {
+	t.Run("SucceedsWithDefaultResponseTemplate", func(t *testing.T) {
+		tmpl, err := initResponseBodyTemplate(ResponseTemplate)
+
+		assert.NilError(t, err)
+		assert.Assert(t, tmpl != nil)
+	})
+
+	t.Run("ErrorOnMalformedTemplate", func(t *testing.T) {
+		bogusTemplate := "{{}{{bogus}}}"
+
+		tmpl, err := initResponseBodyTemplate(bogusTemplate)
+
+		assert.Assert(t, is.Nil(tmpl))
+		assert.ErrorContains(t, err, "parsing response body template failed")
+	})
+
+	t.Run("ErrorOnTemplateWithUnexpectedParams", func(t *testing.T) {
+		bogusTemplate := "{{.Bogus}}"
+
+		tmpl, err := initResponseBodyTemplate(bogusTemplate)
+
+		assert.Assert(t, is.Nil(tmpl))
+		assert.ErrorContains(t, err, "executing response body template failed")
+	})
 }
 
 func TestNewHandler(t *testing.T) {
 	f := newFixture()
 
-	t.Run("SetsUnsubscribeAddress", func(t *testing.T) {
+	t.Run("SetsBasicFields", func(t *testing.T) {
 		assert.Equal(t, testUnsubscribeAddress, f.h.UnsubscribeAddr)
+		assert.Equal(t, testSiteTitle, f.h.SiteTitle)
+		assert.Assert(t, f.h.responseTemplate != nil)
 	})
 
 	t.Run("SetsRedirectMap", func(t *testing.T) {
@@ -91,6 +129,17 @@ func TestNewHandler(t *testing.T) {
 
 		assert.DeepEqual(t, expected, f.h.Redirects)
 	})
+
+	t.Run("ReturnsErrorIfTemplateFailsToParse", func(t *testing.T) {
+		tmpl := "{{.Bogus}}"
+
+		handler, err := NewHandler(
+			testEmailDomain, testSiteTitle, &testAgent{}, testRedirects, tmpl,
+		)
+
+		assert.Assert(t, is.Nil(handler))
+		assert.ErrorContains(t, err, "response body template failed")
+	})
 }
 
 func captureLogs() (*strings.Builder, func()) {
@@ -119,7 +168,58 @@ func apiGatewayRequest(method, path string) *events.APIGatewayV2HTTPRequest {
 }
 
 func apiGatewayResponse(status int) *events.APIGatewayV2HTTPResponse {
-	return &events.APIGatewayV2HTTPResponse{StatusCode: status}
+	return &events.APIGatewayV2HTTPResponse{
+		StatusCode: status, Headers: map[string]string{},
+	}
+}
+
+func TestAddResponseBody(t *testing.T) {
+	const body = "<p>This is only a test</p>"
+	f := newFixture()
+	res := apiGatewayResponse(http.StatusOK)
+
+	t.Run("AddsHtmlBody", func(t *testing.T) {
+		f.h.addResponseBody(res, body)
+
+		assert.Equal(t, res.Headers["content-type"], "text/html; charset=utf-8")
+		assert.Assert(t, is.Contains(res.Body, body))
+		assert.Assert(t, is.Contains(res.Body, "200 OK - "+testSiteTitle))
+	})
+
+	t.Run("FallsBackToTextBodyOnError", func(t *testing.T) {
+		tmpl := template.Must(template.New("bogus").Parse("{{.Bogus}}"))
+		f.h.responseTemplate = tmpl
+		logs, teardown := captureLogs()
+		defer teardown()
+
+		f.h.addResponseBody(res, body)
+
+		assert.Equal(t, res.Headers["content-type"], "text/plain; charset=utf-8")
+		assert.Assert(t, is.Contains(res.Body, "This is only a test"))
+		assert.Assert(t, is.Contains(res.Body, "200 OK - "+testSiteTitle))
+		expected := "ERROR adding HTML response body:"
+		assert.Assert(t, is.Contains(logs.String(), expected))
+	})
+}
+
+func TestErrorResponse(t *testing.T) {
+	f := newFixture()
+
+	t.Run("ReturnInternalServerErrorByDefault", func(t *testing.T) {
+		res := f.h.errorResponse(fmt.Errorf("bad news..."))
+
+		assert.Equal(t, res.StatusCode, http.StatusInternalServerError)
+		assert.Assert(t, is.Contains(res.Body, "There was a problem on our end"))
+	})
+
+	t.Run("ReturnStatusFromError", func(t *testing.T) {
+		err := &errorWithStatus{http.StatusBadGateway, "not our fault..."}
+
+		res := f.h.errorResponse(err)
+
+		assert.Equal(t, res.StatusCode, http.StatusBadGateway)
+		assert.Assert(t, is.Contains(res.Body, "There was a problem on our end"))
+	})
 }
 
 func TestLogApiResponse(t *testing.T) {
@@ -310,7 +410,7 @@ func TestHandleApiEvent(t *testing.T) {
 		"content-type": "application/x-www-form-urlencoded",
 	}
 
-	t.Run("SetsInternalServerErrorByDefault", func(t *testing.T) {
+	t.Run("ReturnsErrorIfNewApiRequestFails", func(t *testing.T) {
 		f := newFixture()
 		badReq := apiGatewayRequest(http.MethodPost, "/subscribe")
 		defer logs.Reset()
@@ -327,7 +427,7 @@ func TestHandleApiEvent(t *testing.T) {
 		)
 	})
 
-	t.Run("SetsStatusFromErrorIfPresent", func(t *testing.T) {
+	t.Run("ReturnsErrorIfHandleApiRequestFails", func(t *testing.T) {
 		f := newFixture()
 		f.ta.Error = &ops.OperationErrorExternal{Message: "db operation failed"}
 		defer logs.Reset()

diff --git a/handler/options.go b/handler/options.go
@@ -17,6 +17,7 @@ type Options struct {
 	ApiDomainName        string
 	ApiMappingKey        string
 	EmailDomainName      string
+	EmailSiteTitle       string
 	SenderName           string
 	SubscribersTableName string
 
@@ -47,6 +48,7 @@ func (env *environment) options() (*Options, error) {
 	env.assign(&opts.ApiDomainName, "API_DOMAIN_NAME")
 	env.assign(&opts.ApiMappingKey, "API_MAPPING_KEY")
 	env.assign(&opts.EmailDomainName, "EMAIL_DOMAIN_NAME")
+	env.assign(&opts.EmailSiteTitle, "EMAIL_SITE_TITLE")
 	env.assign(&opts.SenderName, "SENDER_NAME")
 	env.assign(&opts.SubscribersTableName, "SUBSCRIBERS_TABLE_NAME")
 

diff --git a/handler/options_test.go b/handler/options_test.go
@@ -25,6 +25,7 @@ func TestReportUndefinedEnviromentVariables(t *testing.T) {
 				"API_DOMAIN_NAME",
 				"API_MAPPING_KEY",
 				"EMAIL_DOMAIN_NAME",
+				"EMAIL_SITE_TITLE",
 				"SENDER_NAME",
 				"SUBSCRIBERS_TABLE_NAME",
 				"INVALID_REQUEST_PATH",
@@ -43,6 +44,7 @@ func TestAllRequiredEnvironmentVariablesDefined(t *testing.T) {
 		"API_DOMAIN_NAME":         "api.mike-bland.com",
 		"API_MAPPING_KEY":         "email",
 		"EMAIL_DOMAIN_NAME":       "mike-bland.com",
+		"EMAIL_SITE_TITLE":        "Mike Bland's blog",
 		"SENDER_NAME":             "Mike Bland",
 		"SUBSCRIBERS_TABLE_NAME":  "subscribers",
 		"INVALID_REQUEST_PATH":    "/invalid",
@@ -64,6 +66,7 @@ func TestAllRequiredEnvironmentVariablesDefined(t *testing.T) {
 			ApiDomainName:        "api.mike-bland.com",
 			ApiMappingKey:        "email",
 			EmailDomainName:      "mike-bland.com",
+			EmailSiteTitle:       "Mike Bland's blog",
 			SenderName:           "Mike Bland",
 			SubscribersTableName: "subscribers",
 

diff --git a/lambda/main.go b/lambda/main.go
@@ -21,13 +21,15 @@ func buildHandler() (*handler.Handler, error) {
 	} else {
 		return handler.NewHandler(
 			opts.EmailDomainName,
+			opts.EmailSiteTitle,
 			&ops.ProdAgent{
 				Db:        db.NewDynamoDb(cfg, opts.SubscribersTableName),
 				Validator: email.AddressValidatorImpl{},
 				Mailer:    email.NewSesMailer(cfg),
 			},
 			opts.RedirectPaths,
-		), nil
+			handler.ResponseTemplate,
+		)
 	}
 }
 

diff --git a/template.yml b/template.yml
@@ -11,6 +11,8 @@ Parameters:
     Type: String
   EmailDomainName:
     Type: String
+  EmailSiteTitle:
+    Type: String
   SenderName:
     Type: String
   SubscribersTableName:
@@ -55,6 +57,7 @@ Resources:
           API_DOMAIN_NAME: !Ref ApiDomainName
           API_MAPPING_KEY: !Ref ApiMappingKey
           EMAIL_DOMAIN_NAME: !Ref EmailDomainName
+          EMAIL_SITE_TITLE: !Ref EmailSiteTitle
           SENDER_NAME: !Ref SenderName
           SUBSCRIBERS_TABLE_NAME: !Ref SubscribersTableName
           INVALID_REQUEST_PATH: !Ref InvalidRequestPath