bot startup resilience

base/server.go

@@ -67,6 +67,24 @@ func (s *Server) Name() string {
 	return s.name
 }
 
+// RunBot runs a bot to completion and reports any fatal error to stdout and,
+// when Start has configured chat error reporting, to the error conversation.
+func RunBot(s *Server, run func() error) int {
+	if err := run(); err != nil {
+		s.ReportFatalError(err)
+		return 3
+	}
+	return 0
+}
+
+func (s *Server) ReportFatalError(err error) {
+	fmt.Printf("error running chat loop: %v\n", err)
+	if s.DebugOutput == nil {
+		return
+	}
+	s.Report("```fatal error running %s: %v```", s.name, err)
+}
+
 func (s *Server) SetBotAdmins(admins []string) {
 	s.botAdmins = admins
 }

canarybot/main.go

@@ -101,9 +101,5 @@ func mainInner() int {
 		return 3
 	}
 	bs := NewBotServer(*opts)
-	if err := bs.Go(); err != nil {
-		fmt.Printf("error running chat loop: %s\n", err)
-		return 3
-	}
-	return 0
+	return base.RunBot(bs.Server, bs.Go)
 }

elastiwatch/main.go

@@ -206,9 +206,5 @@ func mainInner() int {
 	opts.AlertConvID = chat1.ConvIDStr(alertConvID)
 	opts.EmailConvID = chat1.ConvIDStr(emailConvID)
 	bs := NewBotServer(*opts)
-	if err := bs.Go(); err != nil {
-		fmt.Printf("error running chat loop: %v\n", err)
-		return 3
-	}
-	return 0
+	return base.RunBot(bs.Server, bs.Go)
 }

gcalbot/main.go

@@ -259,10 +259,5 @@ func mainInner() int {
 		return 3
 	}
 	bs := NewBotServer(*opts)
-
-	if err := bs.Go(); err != nil {
-		fmt.Printf("error running chat loop: %s\n", err)
-		return 3
-	}
-	return 0
+	return base.RunBot(bs.Server, bs.Go)
 }

githubbot/main.go

@@ -293,9 +293,5 @@ func mainInner() int {
 		return 3
 	}
 	bs := NewBotServer(*opts)
-	if err := bs.Go(); err != nil {
-		fmt.Printf("error running chat loop: %v\n", err)
-		return 3
-	}
-	return 0
+	return base.RunBot(bs.Server, bs.Go)
 }

gitlabbot/db.sql

@@ -2,5 +2,6 @@ CREATE TABLE `subscriptions` (
   `conv_id` char(64) NOT NULL,
   `repo` varchar(128) NOT NULL,
   `oauth_identifier` varchar(128) NOT NULL,
+  `reauthorization_needed` boolean NOT NULL DEFAULT false,
   UNIQUE KEY unique_subscription (`conv_id`, `repo`)
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;

gitlabbot/gitlabbot/db.go

@@ -3,6 +3,7 @@ package gitlabbot
 import (
 	"context"
 	"database/sql"
+	"fmt"
 
 	"github.com/keybase/go-keybase-chat-bot/kbchat/types/chat1"
 
@@ -14,6 +15,11 @@ type DB struct {
 	*base.DB
 }
 
+type SubscribedConv struct {
+	ConvID                chat1.ConvIDStr
+	ReauthorizationNeeded bool
+}
+
 func NewDB(db *sql.DB) *DB {
 	return &DB{
 		DB: base.NewDB(db),
@@ -49,23 +55,22 @@ func (d *DB) DeleteSubscriptionsForRepo(ctx context.Context, convID chat1.ConvID
 	return err
 }
 
-func (d *DB) GetSubscribedConvs(ctx context.Context, repo string) (res []chat1.ConvIDStr, err error) {
+func (d *DB) GetSubscribedConvs(ctx context.Context, repo string) (res []SubscribedConv, err error) {
 	rows, err := d.QueryContext(ctx, `
-		SELECT conv_id
+		SELECT conv_id, reauthorization_needed
 		FROM subscriptions
 		WHERE repo = ?
-		GROUP BY conv_id
 	`, repo)
 	if err != nil {
 		return res, err
 	}
 	defer rows.Close()
 	for rows.Next() {
-		var convID chat1.ConvIDStr
-		if err := rows.Scan(&convID); err != nil {
+		var subscribedConv SubscribedConv
+		if err := rows.Scan(&subscribedConv.ConvID, &subscribedConv.ReauthorizationNeeded); err != nil {
 			return res, err
 		}
-		res = append(res, convID)
+		res = append(res, subscribedConv)
 	}
 	return res, rows.Err()
 }
@@ -89,22 +94,44 @@ func (d *DB) GetSubscriptionExists(ctx context.Context, convID chat1.ConvIDStr,
 	}
 }
 
-func (d *DB) GetSubscriptionForRepoExists(ctx context.Context, convID chat1.ConvIDStr, repo string) (exists bool, err error) {
+func (d *DB) GetSubscriptionForRepoStatus(ctx context.Context, convID chat1.ConvIDStr, repo string) (
+	exists bool, reauthorizationNeeded bool, err error,
+) {
 	row := d.QueryRowContext(ctx, `
-	SELECT 1
+	SELECT reauthorization_needed
 	FROM subscriptions
 	WHERE (conv_id = ? AND repo = ?)
 	`, convID, repo)
-	var rowRes string
-	err = row.Scan(&rowRes)
+	err = row.Scan(&reauthorizationNeeded)
 	switch err {
 	case sql.ErrNoRows:
-		return false, nil
+		return false, false, nil
 	case nil:
-		return true, nil
+		return true, reauthorizationNeeded, nil
 	default:
-		return false, err
+		return false, false, err
+	}
+}
+
+func (d *DB) CompleteSubscriptionReauthorization(
+	ctx context.Context, convID chat1.ConvIDStr, repo string,
+) error {
+	res, err := d.ExecContext(ctx, `
+		UPDATE subscriptions
+		SET reauthorization_needed = false
+		WHERE conv_id = ? AND repo = ? AND reauthorization_needed = true
+	`, convID, repo)
+	if err != nil {
+		return err
+	}
+	rowsAffected, err := res.RowsAffected()
+	if err != nil {
+		return err
+	}
+	if rowsAffected != 1 {
+		return fmt.Errorf("expected to complete one subscription reauthorization, updated %d", rowsAffected)
 	}
+	return nil
 }
 
 func (d *DB) GetAllSubscriptionsForConvID(ctx context.Context, convID chat1.ConvIDStr) (res []string, err error) {

gitlabbot/gitlabbot/handler.go

@@ -101,13 +101,23 @@ func (h *Handler) handleSubscribe(ctx context.Context, cmd string, msg chat1.Msg
 		return nil
 	}
 
-	alreadyExists, err := h.db.GetSubscriptionForRepoExists(ctx, msg.ConvID, repo)
+	subscriptionFound, reauthorizationNeeded, err := h.db.GetSubscriptionForRepoStatus(ctx, msg.ConvID, repo)
 	if err != nil {
 		return fmt.Errorf("error checking subscription: %s", err)
 	}
 
 	if create {
-		if !alreadyExists {
+		if subscriptionFound && reauthorizationNeeded {
+			_, err = h.kbc.SendMessageByTlfName(msg.Sender.Username, "%s", formatReauthorizationInstructions(repo, hostedURL, msg, h.httpPrefix, h.secret))
+			if err != nil {
+				return fmt.Errorf("error sending message: %s", err)
+			}
+			if !base.IsDirectPrivateMessage(h.kbc.GetUsername(), msg.Sender.Username, msg.Channel) {
+				h.ChatEcho(msg.ConvID, "OK! I've sent instructions to @%s to reauthorize the webhook.", msg.Sender.Username)
+			}
+			return nil
+		}
+		if !subscriptionFound {
 			err = h.db.CreateSubscription(ctx, msg.ConvID, repo, base.IdentifierFromMsg(msg))
 			if err != nil {
 				return fmt.Errorf("error creating subscription: %s", err)
@@ -126,7 +136,7 @@ func (h *Handler) handleSubscribe(ctx context.Context, cmd string, msg chat1.Msg
 		return nil
 	}
 
-	if alreadyExists {
+	if subscriptionFound {
 		err = h.db.DeleteSubscriptionsForRepo(ctx, msg.ConvID, repo)
 		if err != nil {
 			return fmt.Errorf("error deleting subscriptions: %s", err)

gitlabbot/gitlabbot/http.go

@@ -129,12 +129,18 @@ func (h *HTTPSrv) handleWebhook(_ http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	for _, convID := range convs {
-		secretToken := base.MakeSecret(repo, convID, h.secret)
+	for _, conv := range convs {
+		secretToken := base.MakeSecret(repo, conv.ConvID, h.secret)
 		if !hmac.Equal([]byte(signature), []byte(secretToken)) {
-			h.Debug("payload signature mismatch for conversation %s", convID)
+			h.Debug("payload signature mismatch for conversation %s", conv.ConvID)
 			continue
 		}
-		h.ChatEcho(convID, "%s", message)
+		if conv.ReauthorizationNeeded {
+			if err := h.db.CompleteSubscriptionReauthorization(ctx, conv.ConvID, repo); err != nil {
+				h.Errorf("Error completing webhook reauthorization for conversation %s: %s", conv.ConvID, err)
+				continue
+			}
+		}
+		h.ChatEcho(conv.ConvID, "%s", message)
 	}
 }

gitlabbot/gitlabbot/util.go

@@ -75,6 +75,16 @@ Happy coding!`,
 	return message
 }
 
+func formatReauthorizationInstructions(repo string, hostedURL string, msg chat1.MsgSummary, httpAddress string, secret string) string {
+	back := "`"
+	return fmt.Sprintf(`
+To reauthorize notifications, go to %s/%s/hooks and edit the existing webhook for %s%s/gitlabbot/webhook%s.
+Replace its “Secret Token” with %s%s%s.
+
+Happy coding!`,
+		hostedURL, repo, back, httpAddress, back, back, base.MakeSecret(repo, msg.ConvID, secret), back)
+}
+
 // parseRepoInput checks if url or <owner/repo> form
 func parseRepoInput(urlOrRepoPath string) (hostedURL string, repo string, err error) {
 	urlOrRepoPath = strings.TrimSuffix(urlOrRepoPath, ".git")

gitlabbot/gitlabbot/util_test.go

@@ -3,9 +3,26 @@ package gitlabbot
 import (
 	"testing"
 
+	"github.com/keybase/go-keybase-chat-bot/kbchat/types/chat1"
+	"github.com/keybase/managed-bots/base"
 	"github.com/stretchr/testify/require"
 )
 
+func TestFormatReauthorizationInstructions(t *testing.T) {
+	msg := chat1.MsgSummary{ConvID: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"}
+	repo := "owner/repo"
+	hostedURL := "https://gitlab.example.com"
+	httpAddress := "https://bots.example.com"
+	secret := "server-secret"
+
+	res := formatReauthorizationInstructions(repo, hostedURL, msg, httpAddress, secret)
+
+	require.Contains(t, res, "https://gitlab.example.com/owner/repo/hooks")
+	require.Contains(t, res, "edit the existing webhook")
+	require.Contains(t, res, "https://bots.example.com/gitlabbot/webhook")
+	require.Contains(t, res, base.MakeSecret(repo, msg.ConvID, secret))
+}
+
 func TestParseRepoInputWithURL(t *testing.T) {
 	httpPrefix := "https://mywebsite.com"
 	urlRepo := "owner/repo"

gitlabbot/main.go

@@ -192,9 +192,5 @@ func mainInner() int {
 		return 3
 	}
 	bs := NewBotServer(*opts)
-	if err := bs.Go(); err != nil {
-		fmt.Printf("error running chat loop: %v\n", err)
-		return 3
-	}
-	return 0
+	return base.RunBot(bs.Server, bs.Go)
 }

macrobot/main.go

@@ -139,9 +139,5 @@ func mainInner() int {
 		return 3
 	}
 	bs := NewBotServer(*opts)
-	if err := bs.Go(); err != nil {
-		fmt.Printf("error running chat loop: %s\n", err)
-		return 3
-	}
-	return 0
+	return base.RunBot(bs.Server, bs.Go)
 }

meetbot/main.go

@@ -151,9 +151,5 @@ func mainInner() int {
 	}
 
 	bs := NewBotServer(*opts)
-	if err := bs.Go(); err != nil {
-		fmt.Printf("error running chat loop: %s\n", err)
-		return 3
-	}
-	return 0
+	return base.RunBot(bs.Server, bs.Go)
 }

pollbot/main.go

@@ -156,9 +156,5 @@ func mainInner() int {
 		return 3
 	}
 	bs := NewBotServer(*opts)
-	if err := bs.Go(); err != nil {
-		fmt.Printf("error running chat loop: %v\n", err)
-		return 3
-	}
-	return 0
+	return base.RunBot(bs.Server, bs.Go)
 }

triviabot/main.go

@@ -116,9 +116,5 @@ func mainInner() int {
 		return 3
 	}
 	bs := NewBotServer(*opts)
-	if err := bs.Go(); err != nil {
-		fmt.Printf("error running chat loop: %s\n", err)
-		return 3
-	}
-	return 0
+	return base.RunBot(bs.Server, bs.Go)
 }

webhookbot/main.go

@@ -154,9 +154,5 @@ func mainInner() int {
 		return 3
 	}
 	bs := NewBotServer(*opts)
-	if err := bs.Go(); err != nil {
-		fmt.Printf("error running chat loop: %s\n", err)
-		return 3
-	}
-	return 0
+	return base.RunBot(bs.Server, bs.Go)
 }

zoombot/main.go

@@ -186,9 +186,5 @@ func mainInner() int {
 	}
 
 	bs := NewBotServer(*opts)
-	if err := bs.Go(); err != nil {
-		fmt.Printf("error running chat loop: %s\n", err)
-		return 3
-	}
-	return 0
+	return base.RunBot(bs.Server, bs.Go)
 }