diff --git a/backend/app/cmd/server.go b/backend/app/cmd/server.go index 09897ad7..382772df 100644 --- a/backend/app/cmd/server.go +++ b/backend/app/cmd/server.go @@ -438,6 +438,11 @@ func (s *ServerCommand) newServerApp() (*serverApp, error) { SimpleView: s.SimpleView, } + // enable admin notifications only if admin email is set + if s.Notify.Email.AdminNotifications && s.Admin.Shared.Email != "" { + srv.AdminEmail = s.Admin.Shared.Email + } + srv.ScoreThresholds.Low, srv.ScoreThresholds.Critical = s.LowScore, s.CriticalScore var devAuth *provider.DevAuthServer @@ -787,10 +792,6 @@ func (s *ServerCommand) makeNotify(dataStore *service.DataStore, authenticator * return tkn, nil }, } - // enable admin notifications only if admin email is set - if s.Notify.Email.AdminNotifications && s.Admin.Shared.Email != "" { - emailParams.AdminEmail = s.Admin.Shared.Email - } smtpParams := notify.SmtpParams{ Host: s.SMTP.Host, Port: s.SMTP.Port, diff --git a/backend/app/notify/email.go b/backend/app/notify/email.go index 39144492..3edc36dd 100644 --- a/backend/app/notify/email.go +++ b/backend/app/notify/email.go @@ -14,7 +14,6 @@ import ( log "github.com/go-pkgz/lgr" "github.com/go-pkgz/repeater" - "github.com/hashicorp/go-multierror" "github.com/pkg/errors" ) @@ -26,7 +25,6 @@ type EmailParams struct { VerificationTemplate string // verification message template SubscribeURL string // full subscribe handler URL UnsubscribeURL string // full unsubscribe handler URL - AdminEmail string // admin email for sending notifications about new messages TokenGenFn func(userID, email, site string) (string, error) // Unsubscribe token generation function } @@ -242,91 +240,42 @@ func NewEmail(emailParams EmailParams, smtpParams SmtpParams) (*Email, error) { // also sends email to site administrator if appropriate option is set. // Thread safe func (e *Email) Send(ctx context.Context, req Request) (err error) { + if req.Email == "" { + // this means we can't send this request via Email + return nil + } select { case <-ctx.Done(): return errors.Errorf("sending message to %q aborted due to canceled context", req.Email) default: } - - var emails []emailMessage - errs := new(multierror.Error) - - emailMsg, err := e.createUserEmail(req) - errs = multierror.Append(errs, errors.Wrap(err, "problem creating email notification on comment for user")) - if emailMsg != nil { - emails = append(emails, *emailMsg) - } - - emailMsg, err = e.createAdminEmail(req) - errs = multierror.Append(errs, errors.Wrap(err, "problem creating email notification on comment for admin")) - if emailMsg != nil { - emails = append(emails, *emailMsg) - } - - for _, msg := range emails { - err = repeater.NewDefault(5, time.Millisecond*250). - Do(ctx, func() error { return e.sendMessage(msg) }) - errs = multierror.Append(errs, err) - } - return errs.ErrorOrNil() -} - -// construct email for user if it's necessary: -// 1. in case user requested validation message -// 2. in case comment have is a reply and parent message owner subscribed for email notifications -func (e *Email) createUserEmail(req Request) (*emailMessage, error) { - if req.Email == "" { - // this means we can't send this request via Email - return nil, nil - } - var msg string - var err error if req.Verification.Token != "" { log.Printf("[DEBUG] send verification via %s, user %s", e, req.Verification.User) msg, err = e.buildVerificationMessage(req.Verification.User, req.Email, req.Verification.Token, req.Verification.SiteID) if err != nil { - return nil, err + return err } } if req.Comment.ID != "" { - if req.parent.User.ID == req.Comment.User.ID { + if req.parent.User.ID == req.Comment.User.ID && !req.ForAdmin { // don't send anything if if user replied to their own comment - return nil, nil + return nil } log.Printf("[DEBUG] send notification via %s, comment id %s", e, req.Comment.ID) - msg, err = e.buildMessageFromRequest(req, false) + msg, err = e.buildMessageFromRequest(req, req.ForAdmin) if err != nil { - return nil, err + return err } } - return &emailMessage{from: e.From, to: req.Email, message: msg}, nil -} - -// construct email for admin if it's enabled -func (e *Email) createAdminEmail(req Request) (*emailMessage, error) { - if req.Verification.Token != "" { - // don't notify admin on verification request - return nil, nil - } - if e.AdminEmail == "" { - // don't send anything if notifications to admin are disabled - return nil, nil - } - - var msg string - var err error - - log.Printf("[DEBUG] send admin notification via %s, comment id %s", e, req.Comment.ID) - msg, err = e.buildMessageFromRequest(req, true) - if err != nil { - return nil, err - } - - return &emailMessage{from: e.From, to: e.AdminEmail, message: msg}, nil + return repeater.NewDefault(5, time.Millisecond*250).Do( + ctx, + func() error { + return e.sendMessage(emailMessage{from: e.From, to: req.Email, message: msg}) + }) } // buildVerificationMessage generates verification email message based on given input @@ -365,11 +314,6 @@ func (e *Email) buildMessageFromRequest(req Request, forAdmin bool) (string, err unsubscribeLink = "" } - email := req.Email - if forAdmin { - email = e.AdminEmail - } - commentUrlPrefix := req.Comment.Locator.URL + uiNav msg := bytes.Buffer{} tmplData := msgTmplData{ @@ -379,7 +323,7 @@ func (e *Email) buildMessageFromRequest(req Request, forAdmin bool) (string, err CommentLink: commentUrlPrefix + req.Comment.ID, CommentDate: req.Comment.Timestamp, PostTitle: req.Comment.PostTitle, - Email: email, + Email: req.Email, UnsubscribeLink: unsubscribeLink, ForAdmin: forAdmin, } @@ -395,7 +339,7 @@ func (e *Email) buildMessageFromRequest(req Request, forAdmin bool) (string, err if err != nil { return "", errors.Wrapf(err, "error executing template to build comment reply message") } - return e.buildMessage(subject, msg.String(), email, "text/html", unsubscribeLink) + return e.buildMessage(subject, msg.String(), req.Email, "text/html", unsubscribeLink) } // buildMessage generates email message to send using net/smtp.Data() diff --git a/backend/app/notify/email_test.go b/backend/app/notify/email_test.go index 5a441c54..0f68546a 100644 --- a/backend/app/notify/email_test.go +++ b/backend/app/notify/email_test.go @@ -100,17 +100,15 @@ func TestEmailSendErrors(t *testing.T) { e.verifyTmpl, err = template.New("test").Parse("{{.Test}}") assert.NoError(t, err) - err = e.Send(context.Background(), Request{Email: "bad@example.org", Verification: VerificationMetadata{Token: "some"}}) - assert.Error(t, err) - assert.Contains(t, err.Error(), "error executing template to build verification message: template: test:1:2: executing \"test\" at <.Test>: can't evaluate field Test in type notify.verifyTmplData") + assert.EqualError(t, e.Send(context.Background(), Request{Email: "bad@example.org", Verification: VerificationMetadata{Token: "some"}}), + "error executing template to build verification message: template: test:1:2: executing \"test\" at <.Test>: can't evaluate field Test in type notify.verifyTmplData") e.verifyTmpl, err = template.New("test").Parse(defaultEmailVerificationTemplate) assert.NoError(t, err) e.msgTmpl, err = template.New("test").Parse("{{.Test}}") assert.NoError(t, err) - err = e.Send(context.Background(), Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "test"}}, Email: "bad@example.org"}) - assert.Error(t, err) - assert.Contains(t, err.Error(), "error executing template to build comment reply message: template: test:1:2: executing \"test\" at <.Test>: can't evaluate field Test in type notify.msgTmplData") + assert.EqualError(t, e.Send(context.Background(), Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "test"}}, Email: "bad@example.org"}), + "error executing template to build comment reply message: template: test:1:2: executing \"test\" at <.Test>: can't evaluate field Test in type notify.msgTmplData") e.msgTmpl, err = template.New("test").Parse(defaultEmailTemplate) assert.NoError(t, err) @@ -120,9 +118,8 @@ func TestEmailSendErrors(t *testing.T) { "sending message to \"bad@example.org\" aborted due to canceled context") e.smtp = &fakeTestSMTP{} - err = e.Send(context.Background(), Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "error"}}, Email: "bad@example.org"}) - assert.Error(t, err) - assert.Contains(t, err.Error(), "error creating token for unsubscribe link: token generation error") + assert.EqualError(t, e.Send(context.Background(), Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "error"}}, Email: "bad@example.org"}), + "error creating token for unsubscribe link: token generation error") e.msgTmpl, err = template.New("test").Parse(defaultEmailTemplate) assert.NoError(t, err) } @@ -199,7 +196,7 @@ func TestEmail_Send(t *testing.T) { assert.Equal(t, 1, fakeSmtp.readQuitCount()) assert.Equal(t, "test@example.org", fakeSmtp.readRcpt()) // test buildMessageFromRequest separately for message text - res, err := email.buildMessageFromRequest(req, false) + res, err := email.buildMessageFromRequest(req, req.ForAdmin) assert.NoError(t, err) assert.Contains(t, res, `From: from@example.org To: test@example.org @@ -210,39 +207,15 @@ Content-Type: text/html; charset="UTF-8" List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Date: `) -} -func TestEmail_SendAdmin(t *testing.T) { - email, err := NewEmail(EmailParams{From: "from@example.org", AdminEmail: "admin@example.org"}, SmtpParams{}) - assert.NoError(t, err) - assert.NotNil(t, email) - fakeSmtp := fakeTestSMTP{} - email.smtp = &fakeSmtp - email.TokenGenFn = TokenGenFn - req := Request{ - Comment: store.Comment{ID: "999", User: store.User{ID: "1", Name: "test_user"}, ParentID: "1", PostTitle: "test_title"}, - parent: store.Comment{ID: "1", User: store.User{ID: "999", Name: "parent_user"}}, - } - assert.NoError(t, email.Send(context.TODO(), req)) - assert.Equal(t, "from@example.org", fakeSmtp.readMail()) - assert.Equal(t, 1, fakeSmtp.readQuitCount()) - assert.Equal(t, "admin@example.org", fakeSmtp.readRcpt()) - // test buildMessageFromRequest separately for message text - res, err := email.buildMessageFromRequest(req, true) - assert.NoError(t, err) - assert.Contains(t, res, `From: from@example.org -To: admin@example.org -Subject: New comment to your site for "test_title" -Content-Transfer-Encoding: quoted-printable -MIME-version: 1.0 -Content-Type: text/html; charset="UTF-8" -Date: `) // send email to admin without parent set req = Request{ - Comment: store.Comment{ID: "999", User: store.User{ID: "1", Name: "test_user"}, PostTitle: "test_title"}, + Comment: store.Comment{ID: "999", User: store.User{ID: "1", Name: "test_user"}, PostTitle: "test_title"}, + Email: "admin@example.org", + ForAdmin: true, } assert.NoError(t, email.Send(context.TODO(), req)) - res, err = email.buildMessageFromRequest(req, true) + res, err = email.buildMessageFromRequest(req, req.ForAdmin) assert.NoError(t, err) assert.Contains(t, res, `From: from@example.org To: admin@example.org diff --git a/backend/app/notify/notify.go b/backend/app/notify/notify.go index 66de0cde..d8019908 100644 --- a/backend/app/notify/notify.go +++ b/backend/app/notify/notify.go @@ -37,9 +37,11 @@ type Store interface { // Request notification either about comment or about particular user verification type Request struct { - Comment store.Comment // if set sent notifications about new comment - parent store.Comment // fetched only in case Comment is set - Email string // if set (also) send email + Comment store.Comment // if set sent notifications about new comment + parent store.Comment // fetched only in case Comment is set + Email string // if set (also) send email + ForAdmin bool // if set, message supposed to be sent to administrator + Verification VerificationMetadata // if set sent verification notification } @@ -82,9 +84,13 @@ func (s *Service) Submit(req Request) { if s.dataService != nil && req.Comment.ParentID != "" { if p, err := s.dataService.Get(req.Comment.Locator, req.Comment.ParentID, store.User{}); err == nil { req.parent = p - req.Email, err = s.dataService.GetUserEmail(req.Comment.Locator.SiteID, p.User.ID) - if err != nil { - log.Printf("[WARN] can't read email for %s, %v", p.User.ID, err) + // user notification, should fetch email for it. + // administrator notification comes with pre-set email + if req.Email == "" { + req.Email, err = s.dataService.GetUserEmail(req.Comment.Locator.SiteID, p.User.ID) + if err != nil { + log.Printf("[WARN] can't read email for %s, %v", p.User.ID, err) + } } } } diff --git a/backend/app/notify/telegram.go b/backend/app/notify/telegram.go index 0151012b..9c4a650e 100644 --- a/backend/app/notify/telegram.go +++ b/backend/app/notify/telegram.go @@ -90,6 +90,11 @@ func (t *Telegram) Send(ctx context.Context, req Request) error { // verification request received, send nothing return nil } + if req.ForAdmin { + // request for administrator received, do nothing with it + // as we already sent message on request without this flag set + return nil + } client := http.Client{Timeout: telegramTimeOut} log.Printf("[DEBUG] send telegram notification to %s, comment id %s", t.channelID, req.Comment.ID) diff --git a/backend/app/rest/api/rest.go b/backend/app/rest/api/rest.go index 580d9acd..0a1703c2 100644 --- a/backend/app/rest/api/rest.go +++ b/backend/app/rest/api/rest.go @@ -50,6 +50,7 @@ type Rest struct { AnonVote bool WebRoot string RemarkURL string + AdminEmail string ReadOnlyAge int SharedSecret string ScoreThresholds struct { @@ -364,6 +365,7 @@ func (s *Rest) controllerGroups() (public, private, admin, rss) { authenticator: s.Authenticator, notifyService: s.NotifyService, remarkURL: s.RemarkURL, + adminEmail: s.AdminEmail, anonVote: s.AnonVote, } diff --git a/backend/app/rest/api/rest_private.go b/backend/app/rest/api/rest_private.go index 05e22d8e..ce15fcd4 100644 --- a/backend/app/rest/api/rest_private.go +++ b/backend/app/rest/api/rest_private.go @@ -39,6 +39,7 @@ type private struct { notifyService *notify.Service authenticator *auth.Service remarkURL string + adminEmail string anonVote bool } @@ -131,9 +132,14 @@ func (s *private) createCommentCtrl(w http.ResponseWriter, r *http.Request) { s.cache.Flush(cache.Flusher(comment.Locator.SiteID). Scopes(comment.Locator.URL, lastCommentsScope, comment.User.ID, comment.Locator.SiteID)) + // user notification if s.notifyService != nil { s.notifyService.Submit(notify.Request{Comment: finalComment}) } + // admin notification + if s.notifyService != nil && s.adminEmail != "" { + s.notifyService.Submit(notify.Request{Comment: finalComment, Email: s.adminEmail, ForAdmin: true}) + } log.Printf("[DEBUG] created commend %+v", finalComment) diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go index fc19eb81..d3dae176 100644 --- a/backend/app/rest/api/rest_private_test.go +++ b/backend/app/rest/api/rest_private_test.go @@ -596,11 +596,12 @@ func TestRest_EmailNotification(t *testing.T) { parentComment := store.Comment{} require.NoError(t, render.DecodeJSON(strings.NewReader(string(body)), &parentComment)) // wait for mock notification Submit to kick off - time.Sleep(time.Millisecond * 5) - require.Equal(t, 1, len(mockDestination.Get())) - assert.Equal(t, "", mockDestination.Get()[0].Email) + time.Sleep(time.Millisecond * 30) + require.Equal(t, 2, len(mockDestination.Get())) + assert.Empty(t, mockDestination.Get()[0].Email) + assert.Equal(t, "admin@example.org", mockDestination.Get()[1].Email) - // create child comment from another user, no email notification expected + // create child comment from another user, email notification only to admin expected req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment", strings.NewReader(fmt.Sprintf( `{"text": "test 456", "pid": "%s", @@ -615,9 +616,10 @@ func TestRest_EmailNotification(t *testing.T) { require.NoError(t, err) require.Equal(t, http.StatusCreated, resp.StatusCode, string(body)) // wait for mock notification Submit to kick off - time.Sleep(time.Millisecond * 5) - require.Equal(t, 2, len(mockDestination.Get())) - assert.Empty(t, mockDestination.Get()[1].Email) + time.Sleep(time.Millisecond * 30) + require.Equal(t, 4, len(mockDestination.Get())) + assert.Empty(t, mockDestination.Get()[2].Email) + assert.Equal(t, "admin@example.org", mockDestination.Get()[3].Email) // send confirmation token for email req, err = http.NewRequest(http.MethodPost, ts.URL+"/api/v1/email/subscribe?site=remark42&address=good@example.com", nil) @@ -629,10 +631,10 @@ func TestRest_EmailNotification(t *testing.T) { require.NoError(t, err) require.Equal(t, http.StatusOK, resp.StatusCode, string(body)) // wait for mock notification Submit to kick off - time.Sleep(time.Millisecond * 5) - require.Equal(t, 3, len(mockDestination.Get())) - require.NotEmpty(t, mockDestination.Get()[2].Verification) - verificationToken := mockDestination.Get()[2].Verification.Token + time.Sleep(time.Millisecond * 30) + require.Equal(t, 5, len(mockDestination.Get())) + require.NotEmpty(t, mockDestination.Get()[4].Verification) + verificationToken := mockDestination.Get()[4].Verification.Token // verify email req, err = http.NewRequest(http.MethodPost, ts.URL+fmt.Sprintf("/api/v1/email/confirm?site=remark42&tkn=%s", verificationToken), nil) @@ -674,9 +676,9 @@ func TestRest_EmailNotification(t *testing.T) { require.NoError(t, err) require.Equal(t, http.StatusCreated, resp.StatusCode, string(body)) // wait for mock notification Submit to kick off - time.Sleep(time.Millisecond * 5) - require.Equal(t, 4, len(mockDestination.Get())) - assert.Equal(t, "good@example.com", mockDestination.Get()[3].Email) + time.Sleep(time.Millisecond * 30) + require.Equal(t, 7, len(mockDestination.Get())) + assert.Equal(t, "good@example.com", mockDestination.Get()[5].Email) // delete user's email req, err = http.NewRequest(http.MethodDelete, ts.URL+"/api/v1/email?site=remark42", nil) @@ -688,7 +690,7 @@ func TestRest_EmailNotification(t *testing.T) { require.NoError(t, err) assert.Equal(t, http.StatusOK, resp.StatusCode, string(body)) - // create child comment from another user, no email notification expected + // create child comment from another user, no email notification expected except for admin req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment", strings.NewReader( `{"text": "test 321", "user": {"name": "other_user"}, @@ -702,9 +704,9 @@ func TestRest_EmailNotification(t *testing.T) { require.NoError(t, err) require.Equal(t, http.StatusCreated, resp.StatusCode, string(body)) // wait for mock notification Submit to kick off - time.Sleep(time.Millisecond * 5) - require.Equal(t, 5, len(mockDestination.Get())) - assert.Empty(t, mockDestination.Get()[4].Email) + time.Sleep(time.Millisecond * 30) + require.Equal(t, 9, len(mockDestination.Get())) + assert.Empty(t, mockDestination.Get()[7].Email) } func TestRest_UserAllData(t *testing.T) { diff --git a/backend/app/rest/api/rest_test.go b/backend/app/rest/api/rest_test.go index b55a74e9..c8f6d84b 100644 --- a/backend/app/rest/api/rest_test.go +++ b/backend/app/rest/api/rest_test.go @@ -368,9 +368,10 @@ func startupT(t *testing.T) (ts *httptest.Server, srv *Rest, teardown func()) { SecretReader: token.SecretFunc(func() (string, error) { return "secret", nil }), AvatarStore: avatar.NewLocalFS(tmp + "/ava-remark42"), }), - Cache: memCache, - WebRoot: tmp, - RemarkURL: "https://demo.remark42.com", + Cache: memCache, + WebRoot: tmp, + RemarkURL: "https://demo.remark42.com", + AdminEmail: "admin@example.org", ImageService: image.NewService(&image.FileSystem{ Location: tmp + "/pics-remark42", Partitions: 100,