improve telegram notifications

These changes are designed to ease the transition into
the simplified telegram notifications verification model.

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) {

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)
+}

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)

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},