diff --git a/backend/app/cmd/server.go b/backend/app/cmd/server.go index 59082c0e..3b3735e3 100644 --- a/backend/app/cmd/server.go +++ b/backend/app/cmd/server.go @@ -702,7 +702,7 @@ func (s *ServerCommand) makeNotify(dataStore *service.DataStore, authenticator * emailParams := notify.EmailParams{ From: s.Notify.Email.From, VerificationSubject: s.Notify.Email.VerificationSubject, - UnsubscribeURL: s.RemarkURL + "/api/v1/email/unsubscribe", + UnsubscribeURL: s.RemarkURL + "/email/unsubscribe.html", TokenGenFn: func(userID, email, site string) (string, error) { claims := token.Claims{ Handshake: &token.Handshake{ID: userID + "::" + email}, diff --git a/backend/app/rest/api/rest.go b/backend/app/rest/api/rest.go index 535a6c89..34f065dd 100644 --- a/backend/app/rest/api/rest.go +++ b/backend/app/rest/api/rest.go @@ -236,8 +236,6 @@ func (s *Rest) routes() chi.Router { ropen.Get("/list", s.pubRest.listCtrl) ropen.Post("/preview", s.pubRest.previewCommentCtrl) ropen.Get("/info", s.pubRest.infoCtrl) - ropen.Get("/email/unsubscribe", s.privRest.emailUnsubscribeCtrl) - ropen.Post("/email/unsubscribe", s.privRest.emailUnsubscribeCtrl) ropen.Get("/img", s.ImageProxy.Handler) ropen.Route("/rss", func(rrss chi.Router) { @@ -334,6 +332,8 @@ func (s *Rest) routes() chi.Router { rroot.Use(tollbooth_chi.LimitHandler(tollbooth.NewLimiter(50, nil))) rroot.Get("/index.html", s.pubRest.getStartedCtrl) rroot.Get("/robots.txt", s.pubRest.robotsCtrl) + rroot.Get("/email/unsubscribe.html", s.privRest.emailUnsubscribeCtrl) + rroot.Post("/email/unsubscribe.html", s.privRest.emailUnsubscribeCtrl) }) // file server for static content from /web diff --git a/backend/app/rest/api/rest_private.go b/backend/app/rest/api/rest_private.go index 11e30531..8255b1ac 100644 --- a/backend/app/rest/api/rest_private.go +++ b/backend/app/rest/api/rest_private.go @@ -1,10 +1,13 @@ package api import ( + "bytes" "compress/gzip" "encoding/json" "errors" "fmt" + "html/template" + "io" "net/http" "strings" "time" @@ -55,6 +58,21 @@ type privStore interface { Info(locator store.Locator, readonlyAge int) (store.PostInfo, error) } +const unsubscribeHtml = ` + + + + + + +
+

Remark42

+

Successfully unsubscribed

+
+ + +` + // POST /comment - adds comment, resets all immutable fields func (s *private) createCommentCtrl(w http.ResponseWriter, r *http.Request) { @@ -343,29 +361,29 @@ func (s *private) setConfirmedEmailCtrl(w http.ResponseWriter, r *http.Request) render.JSON(w, r, R.JSON{"updated": true, "address": val}) } -// POST/GET /email/unsubscribe?site=siteID&tkn=jwt - unsubscribe the user in token from email notifications +// POST/GET /email/unsubscribe.html?site=siteID&tkn=jwt - unsubscribe the user in token from email notifications func (s *private) emailUnsubscribeCtrl(w http.ResponseWriter, r *http.Request) { tkn := r.URL.Query().Get("tkn") if tkn == "" { - rest.SendErrorJSON(w, r, http.StatusBadRequest, errors.New("missing parameter"), "token parameter is required", rest.ErrInternal) + rest.SendErrorHTML(w, r, http.StatusBadRequest, errors.New("missing parameter"), "token parameter is required", rest.ErrInternal) return } locator := store.Locator{SiteID: r.URL.Query().Get("site")} confClaims, err := s.authenticator.TokenService().Parse(tkn) if err != nil { - rest.SendErrorJSON(w, r, http.StatusForbidden, err, "failed to verify confirmation token", rest.ErrInternal) + rest.SendErrorHTML(w, r, http.StatusForbidden, err, "failed to verify confirmation token", rest.ErrInternal) return } if s.authenticator.TokenService().IsExpired(confClaims) { - rest.SendErrorJSON(w, r, http.StatusForbidden, errors.New("expired"), "failed to verify confirmation token", rest.ErrInternal) + rest.SendErrorHTML(w, r, http.StatusForbidden, errors.New("expired"), "failed to verify confirmation token", rest.ErrInternal) return } elems := strings.Split(confClaims.Handshake.ID, "::") if len(elems) != 2 { - rest.SendErrorJSON(w, r, http.StatusBadRequest, errors.New(confClaims.Handshake.ID), "invalid handshake token", rest.ErrInternal) + rest.SendErrorHTML(w, r, http.StatusBadRequest, errors.New(confClaims.Handshake.ID), "invalid handshake token", rest.ErrInternal) return } userID := elems[0] @@ -376,11 +394,11 @@ func (s *private) emailUnsubscribeCtrl(w http.ResponseWriter, r *http.Request) { log.Printf("[WARN] can't read email for %s, %v", userID, err) } if existingAddress == "" { - rest.SendErrorJSON(w, r, http.StatusConflict, errors.New("user is not subscribed"), "user does not have active email subscription", rest.ErrInternal) + rest.SendErrorHTML(w, r, http.StatusConflict, errors.New("user is not subscribed"), "user does not have active email subscription", rest.ErrInternal) return } if address != existingAddress { - rest.SendErrorJSON(w, r, http.StatusBadRequest, errors.New("wrong email unsubscription"), "email address in request does not match known for this user", rest.ErrInternal) + rest.SendErrorHTML(w, r, http.StatusBadRequest, errors.New("wrong email unsubscription"), "email address in request does not match known for this user", rest.ErrInternal) return } @@ -388,7 +406,7 @@ func (s *private) emailUnsubscribeCtrl(w http.ResponseWriter, r *http.Request) { if err := s.dataService.DeleteUserDetail(locator, userID, engine.UserEmail); err != nil { code := parseError(err, rest.ErrInternal) - rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't delete email for user", code) + rest.SendErrorHTML(w, r, http.StatusBadRequest, err, "can't delete email for user", code) return } // clean User.Email from the token, if user has the token @@ -399,11 +417,22 @@ func (s *private) emailUnsubscribeCtrl(w http.ResponseWriter, r *http.Request) { if claims.User != nil && claims.User.Email != "" { claims.User.Email = "" if _, err = s.authenticator.TokenService().Set(w, claims); err != nil { - rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "failed to set token", rest.ErrInternal) + rest.SendErrorHTML(w, r, http.StatusInternalServerError, err, "failed to set token", rest.ErrInternal) return } } - render.JSON(w, r, R.JSON{"unsubscribed": true}) + + // MustExecute behaves like template.Execute, but panics if an error occurs. + MustExecute := func(tmpl *template.Template, wr io.Writer, data interface{}) { + if err := tmpl.Execute(wr, data); err != nil { + panic(err) + } + } + + tmpl := template.Must(template.New("unsubscribe").Parse(unsubscribeHtml)) + msg := bytes.Buffer{} + MustExecute(tmpl, &msg, nil) + render.HTML(w, r, msg.String()) } // DELETE /email?site=siteID - removes user's email diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go index 8ed68ff1..9a125e56 100644 --- a/backend/app/rest/api/rest_private_test.go +++ b/backend/app/rest/api/rest_private_test.go @@ -543,10 +543,10 @@ func TestRest_Email(t *testing.T) { {description: "delete user email", url: "/api/v1/email?site=remark42", method: http.MethodDelete, responseCode: http.StatusOK}, {description: "send another confirmation", url: "/api/v1/email/subscribe?site=remark42&address=good@example.com", method: http.MethodPost, responseCode: http.StatusOK}, {description: "set user email, token is good", url: fmt.Sprintf("/api/v1/email/confirm?site=remark42&tkn=%s", goodToken), method: http.MethodPost, responseCode: http.StatusOK, cookieEmail: "good@example.com"}, - {description: "unsubscribe user, no token", url: "/api/v1/email/unsubscribe?site=remark42", method: http.MethodPost, responseCode: http.StatusBadRequest}, - {description: "unsubscribe user, wrong token", url: "/api/v1/email/unsubscribe?site=remark42&tkn=jwt", method: http.MethodPost, responseCode: http.StatusForbidden}, - {description: "unsubscribe user, good token", url: fmt.Sprintf("/api/v1/email/unsubscribe?site=remark42&tkn=%s", goodToken), method: http.MethodPost, responseCode: http.StatusOK}, - {description: "unsubscribe user second time, good token", url: fmt.Sprintf("/api/v1/email/unsubscribe?site=remark42&tkn=%s", goodToken), method: http.MethodPost, responseCode: http.StatusConflict}, + {description: "unsubscribe user, no token", url: "/email/unsubscribe.html?site=remark42", method: http.MethodPost, responseCode: http.StatusBadRequest}, + {description: "unsubscribe user, wrong token", url: "/email/unsubscribe.html?site=remark42&tkn=jwt", method: http.MethodGet, responseCode: http.StatusForbidden}, + {description: "unsubscribe user, good token", url: fmt.Sprintf("/email/unsubscribe.html?site=remark42&tkn=%s", goodToken), method: http.MethodPost, responseCode: http.StatusOK}, + {description: "unsubscribe user second time, good token", url: fmt.Sprintf("/email/unsubscribe.html?site=remark42&tkn=%s", goodToken), method: http.MethodPost, responseCode: http.StatusConflict}, } client := http.Client{} for _, x := range testData { diff --git a/backend/app/rest/httperrors.go b/backend/app/rest/httperrors.go index e1539c36..7b2fbae5 100644 --- a/backend/app/rest/httperrors.go +++ b/backend/app/rest/httperrors.go @@ -1,7 +1,10 @@ package rest import ( + "bytes" "fmt" + "html/template" + "io" "net/http" "net/url" "runtime" @@ -35,6 +38,48 @@ const ( ErrAssetNotFound = 18 // requested file not found ) +const errorHtml = ` + + + + + + +
+

Remark42

+

{{.Error}}: {{.Details}}.

+
+ + +` + +// errTmplData store data for error message +type errTmplData struct { + Error string + Details string +} + +// SendErrorHTML makes html body with provided template and responds with provided http status code, +// error code is not included in render as it is intended for UI developers and not for the users +func SendErrorHTML(w http.ResponseWriter, r *http.Request, httpStatusCode int, err error, details string, errCode int) { + // MustExecute behaves like template.Execute, but panics if an error occurs. + MustExecute := func(tmpl *template.Template, wr io.Writer, data interface{}) { + if err := tmpl.Execute(wr, data); err != nil { + panic(err) + } + } + + tmpl := template.Must(template.New("error").Parse(errorHtml)) + log.Printf("[WARN] %s", errDetailsMsg(r, httpStatusCode, err, details, errCode)) + render.Status(r, httpStatusCode) + msg := bytes.Buffer{} + MustExecute(tmpl, &msg, errTmplData{ + Error: err.Error(), + Details: details, + }) + render.HTML(w, r, msg.String()) +} + // SendErrorJSON makes {error: blah, details: blah} json body and responds with error code func SendErrorJSON(w http.ResponseWriter, r *http.Request, httpStatusCode int, err error, details string, errCode int) { log.Printf("[WARN] %s", errDetailsMsg(r, httpStatusCode, err, details, errCode)) diff --git a/backend/app/rest/httperrors_test.go b/backend/app/rest/httperrors_test.go index 320e8dbc..adf0ad98 100644 --- a/backend/app/rest/httperrors_test.go +++ b/backend/app/rest/httperrors_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/umputun/remark/backend/app/store" ) @@ -36,13 +37,41 @@ func TestSendErrorJSON(t *testing.T) { assert.Equal(t, `{"code":123,"details":"error details 123456","error":"error 500"}`+"\n", string(body)) } +func TestSendErrorHTML(t *testing.T) { + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/error" { + t.Log("http err request", r.URL) + SendErrorHTML(w, r, 500, errors.New("error 500"), "error details 123456", 987) + return + } + w.WriteHeader(404) + })) + + defer ts.Close() + + resp, err := http.Get(ts.URL + "/error") + require.Nil(t, err) + defer resp.Body.Close() + + body, err := ioutil.ReadAll(resp.Body) + require.Nil(t, err) + assert.Equal(t, 500, resp.StatusCode) + + assert.NotContains(t, string(body), `987`, "user html should not contain internal error code") + assert.Contains(t, string(body), `error details 123456`) + assert.Contains(t, string(body), `error 500`) +} + func TestErrorDetailsMsg(t *testing.T) { callerFn := func() { req, err := http.NewRequest("GET", "https://example.com/test?k1=v1&k2=v2", nil) require.Nil(t, err) req.RemoteAddr = "1.2.3.4" msg := errDetailsMsg(req, 500, errors.New("error 500"), "error details 123456", 123) - assert.Equal(t, "error details 123456 - error 500 - 500 (123) - https://example.com/test?k1=v1&k2=v2 - [app/rest/httperrors_test.go:47 rest.TestErrorDetailsMsg]", msg) + assert.Contains(t, msg, "error details 123456 - error 500 - 500 (123) - https://example.com/test?k1=v1&k2=v2 - [app/rest/httperrors_test.go:") + // error line in the middle of the message is not checked + assert.Contains(t, msg, " rest.TestErrorDetailsMsg]") } callerFn() } @@ -55,8 +84,9 @@ func TestErrorDetailsMsgWithUser(t *testing.T) { req = SetUserInfo(req, store.User{Name: "test", ID: "id"}) require.Nil(t, err) msg := errDetailsMsg(req, 500, errors.New("error 500"), "error details 123456", 34567) - assert.Equal(t, "error details 123456 - error 500 - 500 (34567) - test/id - https://example."+ - "com/test?k1=v1&k2=v2 - [app/rest/httperrors_test.go:61 rest.TestErrorDetailsMsgWithUser]", msg) + assert.Contains(t, msg, "error details 123456 - error 500 - 500 (34567) - test/id - https://example.com/test?k1=v1&k2=v2 - [app/rest/httperrors_test.go:") + // error line in the middle of the message is not checked + assert.Contains(t, msg, " rest.TestErrorDetailsMsgWithUser]") } callerFn() }