From 6449b7d92b53d0ef5f39b15020c06193b67684c8 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Fri, 29 Oct 2021 18:47:33 +0200 Subject: [PATCH] improve telegram notifications These changes are designed to ease the transition into the simplified telegram notifications verification model. --- backend/app/cmd/server.go | 99 ++++++------- backend/app/notify/telegram.go | 163 +++++++++++----------- backend/app/notify/telegram_test.go | 74 +++++----- backend/app/rest/api/rest_private_test.go | 4 +- 4 files changed, 175 insertions(+), 165 deletions(-) diff --git a/backend/app/cmd/server.go b/backend/app/cmd/server.go index fc6f0359..0e8e6527 100644 --- a/backend/app/cmd/server.go +++ b/backend/app/cmd/server.go @@ -472,23 +472,26 @@ func (s *ServerCommand) newServerApp(ctx context.Context) (*serverApp, error) { KeyStore: adminStore, } - var emailNotifications bool - notifyService, telegramBotUsername, err := s.makeNotify(dataService, authenticator) - - if contains("email", s.Notify.Users) { - emailNotifications = true - } - - // we pass telegramBotUsername to Rest server only if user notifications are enabled - if !contains("telegram", s.Notify.Users) { - telegramBotUsername = "" - } - + var telegramService *notify.Telegram + var telegramBotUsername string + notifyDestinations, err := s.makeNotifyDestinations(authenticator) if err != nil { - log.Printf("[WARN] failed to make notify service, %s", err) - notifyService = notify.NopService // disable notifier - emailNotifications = false // email notifications are not available in this case - telegramBotUsername = "" // telegram notifications are not available in this case either + log.Printf("[WARN] failed to prepare notify destinations, %s", err) + } + + if contains("telegram", s.Notify.Users) || contains("telegram", s.Notify.Admins) { + telegramService, err = s.makeTelegramNotify() + if err != nil { + log.Printf("[WARN] failed to make telegram notify service, %s", err) + } else { + notifyDestinations = append(notifyDestinations, telegramService) + } + } + + notifyService := s.makeNotifyService(dataService, notifyDestinations) + + if telegramService != nil { + telegramBotUsername = telegramService.GetBotUsername() } imgProxy := &proxy.Image{ @@ -526,7 +529,7 @@ func (s *ServerCommand) newServerApp(ctx context.Context) (*serverApp, error) { SSLConfig: sslConfig, UpdateLimiter: s.UpdateLimit, ImageService: imageService, - EmailNotifications: emailNotifications, + EmailNotifications: contains("email", s.Notify.Users), TelegramBotUsername: telegramBotUsername, EmojiEnabled: s.EnableEmoji, AnonVote: s.AnonymousVote && s.RestrictVoteIP, @@ -912,11 +915,17 @@ func (s *ServerCommand) loadEmailTemplate() (string, error) { return string(file), nil } -// aside from notify.Service and error, returns telegram bot name which will be passed to the frontend -func (s *ServerCommand) makeNotify(dataStore *service.DataStore, authenticator *auth.Service) (*notify.Service, string, error) { - notifyService := notify.NopService - var destinations []notify.Destination - var telegramBotUsername string +func (s *ServerCommand) makeNotifyService(dataStore *service.DataStore, destinations []notify.Destination) *notify.Service { + if len(destinations) > 0 { + log.Printf("[INFO] make notify, for users: %s, for admins: %s", s.Notify.Users, s.Notify.Admins) + return notify.NewService(dataStore, s.Notify.QueueSize, destinations...) + } + return notify.NopService +} + +// constructs list of notify destinations except for telegram, returns empty list in case of error +func (s *ServerCommand) makeNotifyDestinations(authenticator *auth.Service) ([]notify.Destination, error) { + destinations := make([]notify.Destination, 0) if contains("webhook", s.Notify.Admins) { client := &http.Client{Timeout: 5 * time.Second} @@ -932,7 +941,7 @@ func (s *ServerCommand) makeNotify(dataStore *service.DataStore, authenticator * } webhook, err := notify.NewWebhook(client, whParams) if err != nil { - return nil, "", errors.Wrap(err, "failed to create webhook notification destination") + return destinations, errors.Wrap(err, "failed to create webhook notification destination") } destinations = append(destinations, webhook) } @@ -940,29 +949,11 @@ func (s *ServerCommand) makeNotify(dataStore *service.DataStore, authenticator * if contains("slack", s.Notify.Admins) { slack, err := notify.NewSlack(s.Notify.Slack.Token, s.Notify.Slack.Channel) if err != nil { - return nil, "", errors.Wrap(err, "failed to create slack notification destination") + return destinations, errors.Wrap(err, "failed to create slack notification destination") } destinations = append(destinations, slack) } - if contains("telegram", s.Notify.Users) || contains("telegram", s.Notify.Admins) { - if contains("telegram", s.Notify.Admins) && s.Notify.Telegram.Channel == "" { - return nil, "", errors.New("--notify.telegram.channel must be set for admin notifications to work") - } - telegramParams := notify.TelegramParams{ - AdminChannelID: s.Notify.Telegram.Channel, - UserNotifications: contains("telegram", s.Notify.Users), - Token: s.Telegram.Token, - Timeout: s.Telegram.Timeout, - } - tg, err := notify.NewTelegram(telegramParams) - if err != nil { - return nil, "", errors.Wrap(err, "failed to create telegram notification destination") - } - destinations = append(destinations, tg) - telegramBotUsername = tg.BotUsername - } - // with logic below admin notifications enable notifications for users on the backend even if they // are not enabled explicitly, however they won't be visible to the users in the frontend // because api.Rest.EmailNotifications would be set to false. @@ -1004,16 +995,30 @@ func (s *ServerCommand) makeNotify(dataStore *service.DataStore, authenticator * } emailService, err := notify.NewEmail(emailParams, smtpParams) if err != nil { - return nil, "", errors.Wrap(err, "failed to create email notification destination") + return destinations, errors.Wrap(err, "failed to create email notification destination") } destinations = append(destinations, emailService) } - if len(destinations) > 0 { - log.Printf("[INFO] make notify, for users: %s, for admins: %s", s.Notify.Users, s.Notify.Admins) - notifyService = notify.NewService(dataStore, s.Notify.QueueSize, destinations...) + return destinations, nil +} + +// constructs Telegram notify service +func (s *ServerCommand) makeTelegramNotify() (*notify.Telegram, error) { + if contains("telegram", s.Notify.Admins) && s.Notify.Telegram.Channel == "" { + return nil, errors.New("--notify.telegram.channel must be set for admin notifications to work") } - return notifyService, telegramBotUsername, nil + telegramParams := notify.TelegramParams{ + AdminChannelID: s.Notify.Telegram.Channel, + UserNotifications: contains("telegram", s.Notify.Users), + Token: s.Telegram.Token, + Timeout: s.Telegram.Timeout, + } + tg, err := notify.NewTelegram(telegramParams) + if err != nil { + return nil, errors.Wrap(err, "failed to create telegram notification destination") + } + return tg, nil } func (s *ServerCommand) makeSSLConfig() (config api.SSLConfig, err error) { diff --git a/backend/app/notify/telegram.go b/backend/app/notify/telegram.go index 5139f485..b37f8b66 100644 --- a/backend/app/notify/telegram.go +++ b/backend/app/notify/telegram.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "io" "net/http" "strconv" "strings" @@ -23,7 +24,6 @@ type TelegramParams struct { AdminChannelID string // unique identifier for the target chat or username of the target channel (in the format @channelusername) Token string // token for telegram bot API interactions Timeout time.Duration // http client timeout - BotUsername string // filled with bot username after Telegram creation, used in frontend UserNotifications bool // flag which enables user notifications apiPrefix string // changed only in tests @@ -32,6 +32,8 @@ type TelegramParams struct { // Telegram implements notify.Destination for telegram type Telegram struct { TelegramParams + + username string // bot username } // telegramMsg is used to send message trough Telegram bot API @@ -40,12 +42,9 @@ type telegramMsg struct { ParseMode string `json:"parse_mode,omitempty"` } -// TelegramBotInfo structure contains information about telegram bot +// TelegramBotInfo structure contains information about telegram bot, which is used from whole telegram API response type TelegramBotInfo struct { - ID uint64 `json:"id"` - IsBot bool `json:"is_bot"` - FirstName string `json:"first_name"` - Username string `json:"username"` + Username string `json:"username"` } const telegramTimeOut = 5000 * time.Millisecond @@ -66,46 +65,13 @@ func NewTelegram(params TelegramParams) (*Telegram, error) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - err := repeater.NewDefault(5, time.Millisecond*250).Do(ctx, func() error { - client := http.Client{Timeout: res.Timeout} - resp, err := client.Get(fmt.Sprintf("%s%s/getMe", res.apiPrefix, res.Token)) - if err != nil { - return errors.Wrap(err, "can't initialize telegram notifications") - } - defer func() { - if err = resp.Body.Close(); err != nil { - log.Printf("[WARN] can't close request body, %s", err) - } - }() + botInfo, err := res.botInfo(ctx) + if err != nil { + return nil, errors.Wrapf(err, "can't retrieve bot info from Telegram API") + } + res.username = botInfo.Username - if resp.StatusCode != http.StatusOK { - tgErr := struct { - Description string `json:"description"` - }{} - if err = json.NewDecoder(resp.Body).Decode(&tgErr); err == nil { - return errors.Errorf("unexpected telegram API status code %d, error: %q", resp.StatusCode, tgErr.Description) - } - return errors.Errorf("unexpected telegram API status code %d", resp.StatusCode) - } - - tgResp := struct { - OK bool `json:"ok"` - Result TelegramBotInfo - }{} - - if err = json.NewDecoder(resp.Body).Decode(&tgResp); err != nil { - return errors.Wrap(err, "can't decode response") - } - - if !tgResp.OK || !tgResp.Result.IsBot { - return errors.Errorf("unexpected telegram response %+v", tgResp) - } - - res.BotUsername = tgResp.Result.Username - return nil - }) - - return &res, err + return &res, nil } // Send to telegram recipients @@ -141,44 +107,8 @@ func (t *Telegram) sendMessage(ctx context.Context, b []byte, chatID string) err chatID = "@" + chatID // if chatID not a number enforce @ prefix } - u := fmt.Sprintf("%s%s/sendMessage?chat_id=%s&disable_web_page_preview=true", - t.apiPrefix, t.Token, chatID) - r, err := http.NewRequest("POST", u, bytes.NewReader(b)) - if err != nil { - return errors.Wrap(err, "failed to make telegram request") - } - r.Header.Set("Content-Type", "application/json; charset=utf-8") - - client := http.Client{Timeout: t.Timeout} - r = r.WithContext(ctx) - resp, err := client.Do(r) - if err != nil { - return errors.Wrap(err, "failed to get telegram response") - } - defer func() { - if err = resp.Body.Close(); err != nil { - log.Printf("[WARN] can't close request body, %s", err) - } - }() - - if resp.StatusCode != http.StatusOK { - tgErr := struct { - Description string `json:"description"` - }{} - if err = json.NewDecoder(resp.Body).Decode(&tgErr); err == nil { - return errors.Errorf("unexpected telegram API status code %d, error: %q", resp.StatusCode, tgErr.Description) - } - return errors.Errorf("unexpected telegram API status code %d", resp.StatusCode) - } - - tgResp := struct { - OK bool `json:"ok"` - }{} - - if err = json.NewDecoder(resp.Body).Decode(&tgResp); err != nil { - return errors.Wrap(err, "can't decode telegram response") - } - return nil + url := fmt.Sprintf("sendMessage?chat_id=%s&disable_web_page_preview=true", chatID) + return t.request(ctx, url, b, &struct{}{}) } // buildMessage generates message for generic notification about new comment @@ -264,6 +194,11 @@ func (t *Telegram) buildVerificationMessage(user, token, site string) ([]byte, e return b, nil } +// GetBotUsername returns bot username +func (t *Telegram) GetBotUsername() string { + return t.username +} + func (t *Telegram) String() string { result := "telegram" if t.AdminChannelID != "" { @@ -274,3 +209,65 @@ func (t *Telegram) String() string { } return result } + +// botInfo returns info about configured bot +func (t *Telegram) botInfo(ctx context.Context) (*TelegramBotInfo, error) { + var resp = struct { + Result *TelegramBotInfo `json:"result"` + }{} + + err := t.request(ctx, "getMe", nil, &resp) + if err != nil { + return nil, err + } + if resp.Result == nil { + return nil, errors.New("received empty result") + } + + return resp.Result, nil +} + +func (t *Telegram) request(ctx context.Context, method string, b []byte, data interface{}) error { + return repeater.NewDefault(3, time.Millisecond*250).Do(ctx, func() error { + url := fmt.Sprintf("%s%s/%s", t.apiPrefix, t.Token, method) + + var req *http.Request + var err error + if b == nil { + req, err = http.NewRequestWithContext(ctx, "GET", url, nil) + } else { + req, err = http.NewRequestWithContext(ctx, "POST", url, bytes.NewReader(b)) + req.Header.Set("Content-Type", "application/json; charset=utf-8") + } + if err != nil { + return errors.Wrap(err, "failed to create request") + } + + client := http.Client{Timeout: t.Timeout} + resp, err := client.Do(req) + if err != nil { + return errors.Wrap(err, "failed to send request") + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return t.parseError(resp.Body, resp.StatusCode) + } + + if err = json.NewDecoder(resp.Body).Decode(data); err != nil { + return errors.Wrap(err, "failed to decode json response") + } + + return nil + }) +} + +func (t *Telegram) parseError(r io.Reader, statusCode int) error { + tgErr := struct { + Description string `json:"description"` + }{} + if err := json.NewDecoder(r).Decode(&tgErr); err != nil { + return errors.Errorf("unexpected telegram API status code %d", statusCode) + } + return errors.Errorf("unexpected telegram API status code %d, error: %q", statusCode, tgErr.Description) +} diff --git a/backend/app/notify/telegram_test.go b/backend/app/notify/telegram_test.go index 707b5ac9..4e3f5afd 100644 --- a/backend/app/notify/telegram_test.go +++ b/backend/app/notify/telegram_test.go @@ -15,8 +15,7 @@ import ( ) func TestTelegram_New(t *testing.T) { - - ts := mockTelegramServer() + ts := mockTelegramServer(nil) defer ts.Close() tb, err := NewTelegram(TelegramParams{ @@ -29,15 +28,14 @@ func TestTelegram_New(t *testing.T) { assert.Equal(t, tb.Timeout, time.Second*5) assert.Equal(t, "remark_test", tb.AdminChannelID, "@ added") - st := time.Now() _, err = NewTelegram(TelegramParams{ AdminChannelID: "remark_test", - Token: "bad-resp", + Token: "empty-json", apiPrefix: ts.URL + "/", }) - assert.EqualError(t, err, "unexpected telegram response {OK:false Result:{ID:707381019 IsBot:false FirstName:comments_test Username:remark42_test_bot}}") - assert.True(t, time.Since(st) >= 250*5*time.Millisecond) + assert.EqualError(t, err, "can't retrieve bot info from Telegram API: received empty result") + st := time.Now() _, err = NewTelegram(TelegramParams{ AdminChannelID: "remark_test", Token: "non-json-resp", @@ -45,7 +43,8 @@ func TestTelegram_New(t *testing.T) { apiPrefix: ts.URL + "/", }) assert.Error(t, err) - assert.Contains(t, err.Error(), "can't decode response:") + assert.Contains(t, err.Error(), "failed to decode json response:") + assert.True(t, time.Since(st) >= 250*3*time.Millisecond) _, err = NewTelegram(TelegramParams{ AdminChannelID: "remark_test", @@ -53,7 +52,7 @@ func TestTelegram_New(t *testing.T) { Timeout: 2 * time.Second, apiPrefix: ts.URL + "/", }) - assert.EqualError(t, err, "unexpected telegram API status code 404") + assert.EqualError(t, err, "can't retrieve bot info from Telegram API: unexpected telegram API status code 404") _, err = NewTelegram(TelegramParams{ AdminChannelID: "remark_test", @@ -61,7 +60,7 @@ func TestTelegram_New(t *testing.T) { apiPrefix: "http://127.0.0.1:4321/", }) require.Error(t, err) - assert.Contains(t, err.Error(), "can't initialize telegram notifications") + assert.Contains(t, err.Error(), "can't retrieve bot info from Telegram API") assert.Contains(t, err.Error(), "dial tcp 127.0.0.1:4321: connect: connection refused") _, err = NewTelegram(TelegramParams{ @@ -89,8 +88,22 @@ func TestTelegram_New(t *testing.T) { assert.Equal(t, "1234567890", tb.AdminChannelID, "no @ prefix") } +func TestTelegram_GetBotUsername(t *testing.T) { + ts := mockTelegramServer(nil) + defer ts.Close() + + tb, err := NewTelegram(TelegramParams{ + AdminChannelID: "remark_test", + Token: "good-token", + apiPrefix: ts.URL + "/", + }) + assert.NoError(t, err) + assert.NotNil(t, tb) + assert.Equal(t, "remark42_test_bot", tb.GetBotUsername()) +} + func TestTelegram_Send(t *testing.T) { - ts := mockTelegramServer() + ts := mockTelegramServer(nil) defer ts.Close() tb, err := NewTelegram(TelegramParams{ @@ -124,7 +137,15 @@ func TestTelegram_Send(t *testing.T) { UserNotifications: true, apiPrefix: ts.URL + "/", }) + assert.Nil(t, tb) assert.Error(t, err, "should fail") + tb = &Telegram{ + TelegramParams: TelegramParams{ + AdminChannelID: "remark_test", + Token: "non-json-resp", + UserNotifications: true, + apiPrefix: ts.URL + "/", + }} err = tb.Send(context.TODO(), Request{Comment: c, parent: cp, Telegrams: []string{"test_user_channel"}}) require.Error(t, err) assert.Contains(t, err.Error(), "unexpected telegram API status code 404", "send on broken tg") @@ -147,7 +168,7 @@ func TestTelegram_Send(t *testing.T) { } func TestTelegram_SendVerification(t *testing.T) { - ts := mockTelegramServer() + ts := mockTelegramServer(nil) defer ts.Close() tb, err := NewTelegram(TelegramParams{ @@ -182,37 +203,24 @@ func TestTelegram_SendVerification(t *testing.T) { assert.Contains(t, string(res), `secret_`) } -func mockTelegramServer() *httptest.Server { - router := chi.NewRouter() - router.Get("/good-token/getMe", func(w http.ResponseWriter, r *http.Request) { - s := `{"ok": true, +const getMeResp = `{"ok": true, "result": { "first_name": "comments_test", "id": 707381019, "is_bot": true, "username": "remark42_test_bot" }}` - _, _ = w.Write([]byte(s)) + +func mockTelegramServer(_ http.HandlerFunc) *httptest.Server { + router := chi.NewRouter() + router.Get("/good-token/getMe", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(getMeResp)) }) - router.Get("/bad-resp/getMe", func(w http.ResponseWriter, r *http.Request) { - s := `{"ok": false, - "result": { - "first_name": "comments_test", - "id": 707381019, - "is_bot": false, - "username": "remark42_test_bot" - }}` - _, _ = w.Write([]byte(s)) + router.Get("/empty-json/getMe", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{}`)) }) router.Get("/non-json-resp/getMe", func(w http.ResponseWriter, r *http.Request) { - s := `"ok": false, - "result": { - "first_name": "comments_test", - "id": 707381019, - "is_bot": false, - "username": "remark42_test_bot" - ` - _, _ = w.Write([]byte(s)) + _, _ = w.Write([]byte(`not-a-json`)) }) router.Get("/404/getMe", func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(404) diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go index e1af1082..64605ddb 100644 --- a/backend/app/rest/api/rest_private_test.go +++ b/backend/app/rest/api/rest_private_test.go @@ -605,8 +605,8 @@ func TestRest_EmailAndTelegram(t *testing.T) { {description: "issue delete request without site_id", url: "/api/v1/email", method: http.MethodDelete, responseCode: http.StatusBadRequest}, {description: "delete non-existent user email", url: "/api/v1/email?site=remark42", method: http.MethodDelete, responseCode: http.StatusOK}, {description: "set user email, token not set", url: "/api/v1/email/confirm?site=remark42", method: http.MethodPost, responseCode: http.StatusBadRequest}, - {description: "send confirmation without address", url: "/api/v1/email/subscribe?site=remark42", method: http.MethodPost, responseCode: http.StatusBadRequest}, - {description: "send confirmation", url: "/api/v1/email/subscribe?site=remark42&address=good@example.com", method: http.MethodPost, responseCode: http.StatusOK}, + {description: "send email confirmation without address", url: "/api/v1/email/subscribe?site=remark42", method: http.MethodPost, responseCode: http.StatusBadRequest}, + {description: "send email 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: "send confirmation with same address", url: "/api/v1/email/subscribe?site=remark42&address=good@example.com", method: http.MethodPost, responseCode: http.StatusConflict}, {description: "get user email", url: "/api/v1/email?site=remark42", method: http.MethodGet, responseCode: http.StatusOK},