diff --git a/backend/app/notify/email.go b/backend/app/notify/email.go index bef4e41d..e306eddb 100644 --- a/backend/app/notify/email.go +++ b/backend/app/notify/email.go @@ -14,6 +14,7 @@ import ( log "github.com/go-pkgz/lgr" "github.com/go-pkgz/repeater" + "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/umputun/remark42/backend/app/templates" @@ -164,35 +165,55 @@ func (e *Email) setTemplates() error { return nil } -// Send email about comment reply to Request.Email if it's set, -// also sends email to site administrator if appropriate option is set. +// Send email about comment reply to Request.Emails and Request.AdminEmails +// if they're set. // Thread safe func (e *Email) Send(ctx context.Context, req Request) 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) + return errors.Errorf("sending email messages about comment %q aborted due to canceled context", req.Comment.ID) default: } - 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 - } - log.Printf("[DEBUG] send notification via %s, comment id %s", e, req.Comment.ID) - msg, err := e.buildMessageFromRequest(req, req.ForAdmin) - if err != nil { - return err + result := new(multierror.Error) + + // send user notifications + for _, email := range req.Emails { + email := email + log.Printf("[DEBUG] send user notification via %s, comment id %s", e, req.Comment.ID) + msg, err := e.buildMessageFromRequest(req, email, false) + if err != nil { + return err + } + + err = repeater.NewDefault(5, time.Millisecond*250).Do( + ctx, + func() error { + return e.sendMessage(emailMessage{from: e.From, to: email, message: msg}) + }) + + result = multierror.Append(errors.Wrapf(err, "problem sending user email notification to %q", email)) } - return repeater.NewDefault(5, time.Millisecond*250).Do( - ctx, - func() error { - return e.sendMessage(emailMessage{from: e.From, to: req.Email, message: msg}) - }) + // send admin notifications + for _, email := range req.AdminEmails { + email := email + log.Printf("[DEBUG] send admin notification via %s, comment id %s", e, req.Comment.ID) + msg, err := e.buildMessageFromRequest(req, email, true) + if err != nil { + return err + } + + err = repeater.NewDefault(5, time.Millisecond*250).Do( + ctx, + func() error { + return e.sendMessage(emailMessage{from: e.From, to: email, message: msg}) + }) + + result = multierror.Append(errors.Wrapf(err, "problem sending admin email notification to %q", email)) + } + + return result.ErrorOrNil() } // SendVerification email verification VerificationRequest.Email if it's set. @@ -204,7 +225,7 @@ func (e *Email) SendVerification(ctx context.Context, req VerificationRequest) e } select { case <-ctx.Done(): - return errors.Errorf("sending message to %q aborted due to canceled context", req.Email) + return errors.Errorf("sending message to %q aborted due to canceled context", req.User) default: } @@ -239,16 +260,16 @@ func (e *Email) buildVerificationMessage(user, email, token, site string) (strin } // buildMessageFromRequest generates email message based on Request using e.MsgTemplate -func (e *Email) buildMessageFromRequest(req Request, forAdmin bool) (string, error) { +func (e *Email) buildMessageFromRequest(req Request, email string, forAdmin bool) (string, error) { subject := "New reply to your comment" if forAdmin { subject = "New comment to your site" } if req.Comment.PostTitle != "" { - subject += fmt.Sprintf(" for \"%s\"", req.Comment.PostTitle) + subject += fmt.Sprintf(" for %q", req.Comment.PostTitle) } - token, err := e.TokenGenFn(req.parent.User.ID, req.Email, req.Comment.Locator.SiteID) + token, err := e.TokenGenFn(req.parent.User.ID, email, req.Comment.Locator.SiteID) if err != nil { return "", errors.Wrapf(err, "error creating token for unsubscribe link") } @@ -266,7 +287,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: req.Email, + Email: email, UnsubscribeLink: unsubscribeLink, ForAdmin: forAdmin, } @@ -282,7 +303,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(), req.Email, "text/html", unsubscribeLink) + return e.buildMessage(subject, msg.String(), 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 0e0c6993..e6c76019 100644 --- a/backend/app/notify/email_test.go +++ b/backend/app/notify/email_test.go @@ -113,16 +113,16 @@ func TestEmailSendErrors(t *testing.T) { e.msgTmpl, err = template.New("test").Parse("{{.Test}}") assert.NoError(t, err) - 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"}), + assert.EqualError(t, e.Send(context.Background(), Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "test"}}, Emails: []string{"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") ctx, cancel := context.WithCancel(context.Background()) cancel() - assert.EqualError(t, e.Send(ctx, Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "test"}}, Email: "bad@example.org"}), - "sending message to \"bad@example.org\" aborted due to canceled context") + assert.EqualError(t, e.Send(ctx, Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "test"}}, Emails: []string{"bad@example.org"}}), + "sending email messages about comment \"999\" aborted due to canceled context") e.smtp = &fakeTestSMTP{} - 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"}), + assert.EqualError(t, e.Send(context.Background(), Request{Comment: store.Comment{ID: "999"}, parent: store.Comment{User: store.User{ID: "error"}}, Emails: []string{"bad@example.org"}}), "error creating token for unsubscribe link: token generation error") } @@ -136,10 +136,7 @@ func TestEmailSend_ExitConditions(t *testing.T) { // prevent triggering e.autoFlush creation emptyRequest := Request{Comment: store.Comment{ID: "999"}} assert.NoError(t, email.Send(context.Background(), emptyRequest), - "Message without parent comment User.Email is not sent and returns nil") - requestWithEqualUsersWithEmails := Request{Comment: store.Comment{ID: "999"}, Email: "good_example@example.org"} - assert.NoError(t, email.Send(context.Background(), requestWithEqualUsersWithEmails), - "Message with parent comment User equals comment User is not sent and returns nil") + "Message without Emails and AdminEmails is not sent and returns nil") } func TestEmailSendClientError(t *testing.T) { @@ -184,6 +181,15 @@ func TestEmailSendClientError(t *testing.T) { "e.send called without smtpClient set returns error") } +func TestEmail_DefaultTemplates(t *testing.T) { + email, err := NewEmail(EmailParams{}, SMTPParams{}) + assert.Error(t, err) + assert.Nil(t, email) + email, err = NewEmail(EmailParams{VerificationTemplatePath: "testdata/verification.html.tmpl"}, SMTPParams{}) + assert.Error(t, err) + assert.Nil(t, email) +} + func TestEmail_Send(t *testing.T) { email, err := NewEmail(EmailParams{ From: "from@example.org", @@ -199,14 +205,14 @@ func TestEmail_Send(t *testing.T) { 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"}}, - Email: "test@example.org", + Emails: []string{"test@example.org"}, } 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, "test@example.org", fakeSMTP.readRcpt()) // test buildMessageFromRequest separately for message text - res, err := email.buildMessageFromRequest(req, req.ForAdmin) + res, err := email.buildMessageFromRequest(req, req.Emails[0], false) assert.NoError(t, err) assert.Contains(t, res, `From: from@example.org To: test@example.org @@ -218,14 +224,17 @@ List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Date: `) - // send email to admin without parent set + // send email to both user and admin, without parent set req = Request{ - Comment: store.Comment{ID: "999", User: store.User{ID: "1", Name: "test_user"}, PostTitle: "test_title"}, - Email: "admin@example.org", - ForAdmin: true, + Comment: store.Comment{ID: "999", User: store.User{ID: "1", Name: "test_user"}, PostTitle: "test_title"}, + Emails: []string{"test@example.org"}, + AdminEmails: []string{"admin@example.org"}, } assert.NoError(t, email.Send(context.TODO(), req)) - res, err = email.buildMessageFromRequest(req, req.ForAdmin) + assert.Equal(t, "from@example.org", fakeSMTP.readMail()) + assert.Equal(t, 3, fakeSMTP.readQuitCount(), "plus two emails: one for user and one for admin") + assert.Equal(t, "admin@example.org", fakeSMTP.readRcpt()) + res, err = email.buildMessageFromRequest(req, req.AdminEmails[0], true) assert.NoError(t, err) assert.Contains(t, res, `From: from@example.org To: admin@example.org @@ -268,7 +277,7 @@ func TestEmail_SendVerification(t *testing.T) { // VerificationRequest with canceled context ctx, cancel := context.WithCancel(context.TODO()) cancel() - assert.EqualError(t, email.SendVerification(ctx, req), "sending message to \"test@example.org\" aborted due to canceled context") + assert.EqualError(t, email.SendVerification(ctx, req), "sending message to \"test_username\" aborted due to canceled context") // test buildVerificationMessage separately for message text res, err := email.buildVerificationMessage(req.User, req.Email, req.Token, req.SiteID) diff --git a/backend/app/notify/notify.go b/backend/app/notify/notify.go index d64363f7..1488763e 100644 --- a/backend/app/notify/notify.go +++ b/backend/app/notify/notify.go @@ -37,12 +37,12 @@ type Store interface { GetUserEmail(siteID string, userID string) (string, error) } -// Request notification either about comment or about particular user verification +// Request notification either about comment 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 - ForAdmin bool // if set, message supposed to be sent to administrator + Comment store.Comment + parent store.Comment + Emails []string + AdminEmails []string } // VerificationRequest notification for user @@ -82,18 +82,10 @@ func (s *Service) Submit(req Request) { if len(s.destinations) == 0 || atomic.LoadUint32(&s.closed) != 0 { return } - // parent comment is fetched only if comment is present in the 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 - // 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) - } - } + req.Emails = s.getNotificationEmails(req, p) } } select { @@ -103,6 +95,22 @@ func (s *Service) Submit(req Request) { } } +// getNotificationEmails returns list of emails for notifications for provided comment. +// Emails is not added to the returned list in case original message is from the same user as the notification receiver. +func (s *Service) getNotificationEmails(req Request, notifyComment store.Comment) (result []string) { + // add current user email only if the user is not the one who wrote the original comment + if notifyComment.User.ID != req.Comment.User.ID { + email, err := s.dataService.GetUserEmail(req.Comment.Locator.SiteID, notifyComment.User.ID) + if err != nil { + log.Printf("[WARN] can't read email for %s, %v", notifyComment.User.ID, err) + } + if email != "" { + result = append(result, email) + } + } + return result +} + // SubmitVerification to internal channel if not busy, drop if can't send func (s *Service) SubmitVerification(req VerificationRequest) { if len(s.destinations) == 0 || atomic.LoadUint32(&s.closed) != 0 { diff --git a/backend/app/notify/notify_test.go b/backend/app/notify/notify_test.go index 32ed8672..51bc81c6 100644 --- a/backend/app/notify/notify_test.go +++ b/backend/app/notify/notify_test.go @@ -139,6 +139,65 @@ func TestService_WithParent(t *testing.T) { assert.Equal(t, "", destRes[1].parent.ID) } +func TestService_EmailRetrieval(t *testing.T) { + dest := &MockDest{id: 1} + dataStore := &mockStore{data: map[string]store.Comment{}, emailData: map[string]string{}} + + dataStore.data["p1"] = store.Comment{ID: "p1", User: store.User{ID: "u1"}} + dataStore.data["p2"] = store.Comment{ID: "p2", ParentID: "p1", User: store.User{ID: "u1"}} + dataStore.data["p3"] = store.Comment{ID: "p3", ParentID: "p1", User: store.User{ID: "u2"}} + dataStore.data["p4"] = store.Comment{ID: "p4", ParentID: "p3", User: store.User{ID: "u1"}} + dataStore.emailData["u1"] = "u1@example.com" + + s := NewService(dataStore, 1, dest) + assert.NotNil(t, s) + + // one comment, one notification + s.Submit(Request{Comment: dataStore.data["p1"]}) + time.Sleep(time.Millisecond * 110) + + destRes := dest.Get() + require.Equal(t, 1, len(destRes), "one comment notified") + assert.Equal(t, "p1", destRes[0].Comment.ID) + assert.Empty(t, destRes[0].parent) + assert.Empty(t, destRes[0].Emails) + + // reply to the first comment, same comment as one in original comment + s.Submit(Request{Comment: dataStore.data["p2"]}) + time.Sleep(time.Millisecond * 110) + + destRes = dest.Get() + require.Equal(t, 2, len(destRes), "two comment notified") + assert.Equal(t, "p2", destRes[1].Comment.ID) + assert.Equal(t, "p1", destRes[1].parent.ID) + assert.Equal(t, "u1", destRes[1].parent.User.ID) + assert.Empty(t, destRes[1].Emails, "u1 is not notified they are the one who left the comment") + + // another reply to the first comment, another user + s.Submit(Request{Comment: dataStore.data["p3"]}) + time.Sleep(time.Millisecond * 110) + + destRes = dest.Get() + require.Equal(t, 3, len(destRes), "three comment notified") + assert.Equal(t, "p3", destRes[2].Comment.ID) + assert.Equal(t, "p1", destRes[2].parent.ID) + assert.Equal(t, "u1", destRes[2].parent.User.ID) + assert.Equal(t, []string{"u1@example.com"}, destRes[2].Emails) + + // reply to the last comment by another user, should trigger email retrieval error + s.Submit(Request{Comment: dataStore.data["p4"]}) + time.Sleep(time.Millisecond * 110) + + destRes = dest.Get() + require.Equal(t, 4, len(destRes), "four comment notified") + assert.Equal(t, "p4", destRes[3].Comment.ID) + assert.Equal(t, "p3", destRes[3].parent.ID) + assert.Equal(t, "u2", destRes[3].parent.User.ID) + assert.Empty(t, destRes[3].Emails, "no email can be retrieved for u2") + + s.Close() +} + func TestService_Nop(t *testing.T) { s := NopService s.Submit(Request{Comment: store.Comment{}}) @@ -146,7 +205,10 @@ func TestService_Nop(t *testing.T) { assert.Equal(t, uint32(1), atomic.LoadUint32(&s.closed)) } -type mockStore struct{ data map[string]store.Comment } +type mockStore struct { + data map[string]store.Comment + emailData map[string]string +} func (m mockStore) Get(_ store.Locator, id string, _ store.User) (store.Comment, error) { res, ok := m.data[id] @@ -156,6 +218,10 @@ func (m mockStore) Get(_ store.Locator, id string, _ store.User) (store.Comment, return res, nil } -func (m mockStore) GetUserEmail(_, _ string) (string, error) { - return "", errors.New("no such user") +func (m mockStore) GetUserEmail(_, userID string) (string, error) { + email, ok := m.emailData[userID] + if !ok { + return "", errors.New("no such user") + } + return email, nil } diff --git a/backend/app/notify/telegram.go b/backend/app/notify/telegram.go index 6a5ad467..0912f7c8 100644 --- a/backend/app/notify/telegram.go +++ b/backend/app/notify/telegram.go @@ -85,11 +85,6 @@ func NewTelegram(token, channelID string, timeout time.Duration, api string) (*T // Send to telegram channel func (t *Telegram) Send(ctx context.Context, req Request) error { - 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_private.go b/backend/app/rest/api/rest_private.go index 1dee5cc2..e40b3e75 100644 --- a/backend/app/rest/api/rest_private.go +++ b/backend/app/rest/api/rest_private.go @@ -120,12 +120,12 @@ func (s *private) createCommentCtrl(w http.ResponseWriter, r *http.Request) { Scopes(comment.Locator.URL, lastCommentsScope, comment.User.ID, comment.Locator.SiteID)) if s.notifyService != nil { - // user notification - s.notifyService.Submit(notify.Request{Comment: finalComment}) - // admin notification - for _, adminEmail := range s.adminEmail { - s.notifyService.Submit(notify.Request{Comment: finalComment, Email: adminEmail, ForAdmin: true}) - } + s.notifyService.Submit( + notify.Request{ + Comment: finalComment, + AdminEmails: s.adminEmail, + }, + ) } 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 7619afe1..3c71f909 100644 --- a/backend/app/rest/api/rest_private_test.go +++ b/backend/app/rest/api/rest_private_test.go @@ -618,9 +618,9 @@ func TestRest_EmailNotification(t *testing.T) { require.NoError(t, render.DecodeJSON(strings.NewReader(string(body)), &parentComment)) // wait for mock notification Submit to kick off 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) + require.Equal(t, 1, len(mockDestination.Get())) + assert.Empty(t, mockDestination.Get()[0].Emails) + assert.Equal(t, []string{"admin@example.org"}, mockDestination.Get()[0].AdminEmails) // 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( @@ -630,7 +630,7 @@ func TestRest_EmailNotification(t *testing.T) { "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}`, parentComment.ID))) assert.NoError(t, err) - req.Header.Add("X-JWT", devToken) + req.Header.Add("X-JWT", anonToken) resp, err = client.Do(req) assert.NoError(t, err) body, err = ioutil.ReadAll(resp.Body) @@ -639,9 +639,9 @@ func TestRest_EmailNotification(t *testing.T) { require.Equal(t, http.StatusCreated, resp.StatusCode, string(body)) // wait for mock notification Submit to kick off 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) + require.Equal(t, 2, len(mockDestination.Get())) + assert.Empty(t, mockDestination.Get()[1].Emails) + assert.Equal(t, []string{"admin@example.org"}, mockDestination.Get()[1].AdminEmails) // send confirmation token for email req, err = http.NewRequest(http.MethodPost, ts.URL+"/api/v1/email/subscribe?site=remark42&address=good@example.com", nil) @@ -694,7 +694,7 @@ func TestRest_EmailNotification(t *testing.T) { "locator":{"url": "https://radio-t.com/blah1", "site": "remark42"}}`, parentComment.ID))) assert.NoError(t, err) - req.Header.Add("X-JWT", devToken) + req.Header.Add("X-JWT", anonToken) resp, err = client.Do(req) assert.NoError(t, err) body, err = ioutil.ReadAll(resp.Body) @@ -703,9 +703,9 @@ func TestRest_EmailNotification(t *testing.T) { require.Equal(t, http.StatusCreated, resp.StatusCode, string(body)) // wait for mock notification Submit to kick off time.Sleep(time.Millisecond * 30) - require.Equal(t, 6, len(mockDestination.Get())) - assert.Equal(t, "good@example.com", mockDestination.Get()[4].Email) - assert.Equal(t, "admin@example.org", mockDestination.Get()[5].Email) + require.Equal(t, 3, len(mockDestination.Get())) + assert.Equal(t, []string{"good@example.com"}, mockDestination.Get()[2].Emails) + assert.Equal(t, []string{"admin@example.org"}, mockDestination.Get()[2].AdminEmails) // delete user's email req, err = http.NewRequest(http.MethodDelete, ts.URL+"/api/v1/email?site=remark42", nil) @@ -734,8 +734,9 @@ func TestRest_EmailNotification(t *testing.T) { require.Equal(t, http.StatusCreated, resp.StatusCode, string(body)) // wait for mock notification Submit to kick off time.Sleep(time.Millisecond * 30) - require.Equal(t, 8, len(mockDestination.Get())) - assert.Empty(t, mockDestination.Get()[6].Email) + require.Equal(t, 4, len(mockDestination.Get())) + assert.Empty(t, mockDestination.Get()[3].Emails) + assert.Equal(t, []string{"admin@example.org"}, mockDestination.Get()[3].AdminEmails) } func TestRest_UserAllData(t *testing.T) {