From 9d9ac87f866d442fd85014f24635f8792644cd38 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Tue, 2 Nov 2021 16:06:38 -0500 Subject: [PATCH 01/18] add buttons --- notification/slack/channel.go | 110 ++++++++++++++++------------------ 1 file changed, 53 insertions(+), 57 deletions(-) diff --git a/notification/slack/channel.go b/notification/slack/channel.go index 2c629326a4..cc7460380c 100644 --- a/notification/slack/channel.go +++ b/notification/slack/channel.go @@ -3,7 +3,6 @@ package slack import ( "context" "fmt" - "strings" "sync" "time" @@ -235,6 +234,57 @@ func alertLink(ctx context.Context, id int, summary string) string { return fmt.Sprintf("<%s|Alert #%d: %s>", cfg.CallbackURL(path), id, slackutilsx.EscapeMessage(summary)) } +func alertMsgOption(ctx context.Context, callbackID string, id int, summary, details, logEntry string, status alert.Status) slack.MsgOption { + blocks := []slack.Block{ + slack.NewSectionBlock( + slack.NewTextBlockObject("mrkdwn", alertLink(ctx, id, summary), false, false), nil, nil), + } + + var color string + var actions []slack.Block + switch status { + case alert.StatusActive: + color = colorAcked + actions = []slack.Block{ + slack.NewDividerBlock(), + slack.NewActionBlock("alert_response", + slack.NewButtonBlockElement("close", callbackID, slack.NewTextBlockObject("plain_text", "Close", false, false)), + ), + } + case alert.StatusTriggered: + color = colorUnacked + actions = []slack.Block{ + slack.NewDividerBlock(), + slack.NewActionBlock("alert_response", + slack.NewButtonBlockElement("ack", callbackID, slack.NewTextBlockObject("plain_text", "Acknowledge", false, false)), + slack.NewButtonBlockElement("close", callbackID, slack.NewTextBlockObject("plain_text", "Close", false, false)), + ), + } + case alert.StatusClosed: + color = colorClosed + details = "" + } + if details != "" { + blocks = append(blocks, slack.NewSectionBlock( + slack.NewTextBlockObject("mrkdwn", slackutilsx.EscapeMessage(details), false, false), nil, nil), + ) + } + + blocks = append(blocks, + slack.NewContextBlock("", slack.NewTextBlockObject("plain_text", logEntry, false, false)), + ) + if len(actions) > 0 { + blocks = append(blocks, actions...) + } + + return slack.MsgOptionAttachments( + slack.Attachment{ + Color: color, + Blocks: slack.Blocks{BlockSet: blocks}, + }, + ) +} + func (s *ChannelSender) Send(ctx context.Context, msg notification.Message) (*notification.SentMessage, error) { cfg := config.FromContext(ctx) @@ -254,65 +304,11 @@ func (s *ChannelSender) Send(ctx context.Context, msg notification.Message) (*no break } - blocks := []slack.Block{ - slack.NewSectionBlock( - slack.NewTextBlockObject("mrkdwn", alertLink(ctx, t.AlertID, t.Summary), false, false), nil, nil), - } - - if t.Details != "" { - details := "> " + strings.ReplaceAll(slackutilsx.EscapeMessage(t.Details), "\n", "\n> ") - - blocks = append(blocks, slack.NewSectionBlock( - slack.NewTextBlockObject("mrkdwn", details, false, false), nil, nil), - ) - } - blocks = append(blocks, - slack.NewContextBlock("", slack.NewTextBlockObject("plain_text", "Unacknowledged", false, false))) - - opts = append(opts, - slack.MsgOptionAttachments( - slack.Attachment{ - Color: colorUnacked, - Blocks: slack.Blocks{BlockSet: blocks}, - }, - ), - ) + opts = append(opts, alertMsgOption(ctx, t.CallbackID, t.AlertID, t.Summary, t.Details, "Unacknowledged", alert.StatusTriggered)) case notification.AlertStatus: - var color string - var details string - if t.Details != "" { - details = "> " + strings.ReplaceAll(slackutilsx.EscapeMessage(t.Details), "\n", "\n> ") - } - - switch t.NewAlertStatus { - case alert.StatusActive: - color = colorAcked - case alert.StatusTriggered: - color = colorUnacked - case alert.StatusClosed: - color = colorClosed - details = "" - } - - blocks := []slack.Block{ - slack.NewSectionBlock( - slack.NewTextBlockObject("mrkdwn", alertLink(ctx, t.AlertID, t.Summary), false, false), nil, nil), - } - if details != "" { - blocks = append(blocks, slack.NewSectionBlock( - slack.NewTextBlockObject("mrkdwn", details, false, false), nil, nil), - ) - } - blocks = append(blocks, slack.NewContextBlock("", slack.NewTextBlockObject("plain_text", slackutilsx.EscapeMessage(t.LogEntry), false, false))) - opts = append(opts, slack.MsgOptionUpdate(t.OriginalStatus.ProviderMessageID.ExternalID), - slack.MsgOptionAttachments( - slack.Attachment{ - Color: color, - Blocks: slack.Blocks{BlockSet: blocks}, - }, - ), + alertMsgOption(ctx, t.OriginalStatus.ID, t.AlertID, t.Summary, t.Details, t.LogEntry, t.NewAlertStatus), ) case notification.AlertBundle: opts = append(opts, slack.MsgOptionText( From dc7414afa144157beb6e97aecfd4b0d2cd168e53 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Tue, 2 Nov 2021 17:11:23 -0500 Subject: [PATCH 02/18] base interaction --- app/inithttp.go | 2 + engine/backend.go | 4 +- engine/engine.go | 52 +++++++++++++++++++++ notification/namedreceiver.go | 6 +++ notification/receiver.go | 11 ++++- notification/resultreceiver.go | 1 + notification/slack/channel.go | 23 ++++++++-- notification/slack/servemessageaction.go | 58 ++++++++++++++++++++++++ user/store.go | 37 +++++++++++++++ validation/fieldvalidationerror.go | 5 ++ 10 files changed, 192 insertions(+), 7 deletions(-) create mode 100644 notification/slack/servemessageaction.go diff --git a/app/inithttp.go b/app/inithttp.go index 8f45326073..cf7fe38818 100644 --- a/app/inithttp.go +++ b/app/inithttp.go @@ -199,6 +199,8 @@ func (app *App) initHTTP(ctx context.Context) error { mux.HandleFunc("/api/v2/twilio/call", app.twilioVoice.ServeCall) mux.HandleFunc("/api/v2/twilio/call/status", app.twilioVoice.ServeStatusCallback) + mux.HandleFunc("/api/v2/slack/message-action", app.slackChan.ServeMessageAction) + // Legacy (v1) API mapping mux.HandleFunc("/v1/graphql", app.graphql.ServeHTTP) diff --git a/engine/backend.go b/engine/backend.go index 315c3d7d8a..92177bc6ec 100644 --- a/engine/backend.go +++ b/engine/backend.go @@ -59,11 +59,13 @@ func (b *backend) FindOne(ctx context.Context, id string) (*callback, error) { var c callback var alertID sql.NullInt64 var serviceID sql.NullString - err = b.findOne.QueryRowContext(ctx, id).Scan(&c.ID, &alertID, &serviceID, &c.ContactMethodID) + var cmID sql.NullString + err = b.findOne.QueryRowContext(ctx, id).Scan(&c.ID, &alertID, &serviceID, &cmID) if err != nil { return nil, err } c.AlertID = int(alertID.Int64) c.ServiceID = serviceID.String + c.ContactMethodID = cmID.String return &c, nil } diff --git a/engine/engine.go b/engine/engine.go index 63921ae0b3..c1afb4648e 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -3,6 +3,7 @@ package engine import ( "context" "database/sql" + "fmt" "strings" "time" @@ -306,6 +307,57 @@ func (p *Engine) SetSendResult(ctx context.Context, res *notification.SendResult return err } +// ReceiveSubject will process a notification result. +func (p *Engine) ReceiveSubject(ctx context.Context, providerID, subjectID, callbackID string, result notification.Result) error { + ctx, sp := trace.StartSpan(ctx, "Engine.ReceiveSubject") + defer sp.End() + cb, err := p.b.FindOne(ctx, callbackID) + if err != nil { + return err + } + if cb.ServiceID != "" { + ctx = log.WithField(ctx, "ServiceID", cb.ServiceID) + } + if cb.AlertID != 0 { + ctx = log.WithField(ctx, "AlertID", cb.AlertID) + } + + var usr *user.User + permission.SudoContext(ctx, func(ctx context.Context) { + usr, err = p.cfg.UserStore.FindOneBySubject(ctx, providerID, subjectID) + }) + if err != nil { + return fmt.Errorf("failed to find user: %w", err) + } + if usr == nil { + return notification.ErrUnknownSubject + } + + ctx = permission.UserSourceContext(ctx, usr.ID, usr.Role, &permission.SourceInfo{ + Type: permission.SourceTypeNotificationCallback, + ID: callbackID, + }) + + var newStatus alert.Status + switch result { + case notification.ResultAcknowledge: + newStatus = alert.StatusActive + case notification.ResultResolve: + newStatus = alert.StatusClosed + default: + return errors.New("unknown result type") + } + + if cb.AlertID != 0 { + return errors.Wrap(p.am.UpdateStatus(ctx, cb.AlertID, newStatus), "update alert") + } + if cb.ServiceID != "" { + return errors.Wrap(p.am.UpdateStatusByService(ctx, cb.ServiceID, newStatus), "update all alerts") + } + + return errors.New("unknown callback type") +} + // Receive will process a notification result. func (p *Engine) Receive(ctx context.Context, callbackID string, result notification.Result) error { ctx, sp := trace.StartSpan(ctx, "Engine.Receive") diff --git a/notification/namedreceiver.go b/notification/namedreceiver.go index 5aafe1a462..9ed790ec6b 100644 --- a/notification/namedreceiver.go +++ b/notification/namedreceiver.go @@ -40,3 +40,9 @@ func (nr *namedReceiver) Receive(ctx context.Context, callbackID string, result metricRecvTotal.WithLabelValues(nr.ns.destType.String(), result.String()) return nr.r.Receive(ctx, callbackID, result) } + +// Receive implements the Receiver interface by calling the underlying Receiver.ReceiveSubject method. +func (nr *namedReceiver) ReceiveSubject(ctx context.Context, providerID, subjectID, callbackID string, result Result) error { + metricRecvTotal.WithLabelValues(nr.ns.destType.String(), result.String()) + return nr.r.ReceiveSubject(ctx, providerID, subjectID, callbackID, result) +} diff --git a/notification/receiver.go b/notification/receiver.go index 344c852001..4af0a928e3 100644 --- a/notification/receiver.go +++ b/notification/receiver.go @@ -1,6 +1,9 @@ package notification -import "context" +import ( + "context" + "errors" +) // A Receiver processes incoming messages and responses. type Receiver interface { @@ -10,6 +13,9 @@ type Receiver interface { // Receive records a response to a previously sent message. Receive(ctx context.Context, callbackID string, result Result) error + // ReceiveSubject records a response to a previously sent message from a provider/subject (e.g. Slack user). + ReceiveSubject(ctx context.Context, providerID, subjectID, callbackID string, result Result) error + // Start indicates a user has opted-in for notifications to this contact method. Start(context.Context, Dest) error @@ -19,3 +25,6 @@ type Receiver interface { // IsKnownDest checks if the given destination is known/not disabled. IsKnownDest(ctx context.Context, value string) (bool, error) } + +// ErrUnknownSubject is returend from ReceiveSubject when the subject is unknown. +var ErrUnknownSubject = errors.New("unknown subject for that provider") diff --git a/notification/resultreceiver.go b/notification/resultreceiver.go index 0db1c141a2..bc376411bb 100644 --- a/notification/resultreceiver.go +++ b/notification/resultreceiver.go @@ -9,6 +9,7 @@ type ResultReceiver interface { SetSendResult(ctx context.Context, res *SendResult) error Receive(ctx context.Context, callbackID string, result Result) error + ReceiveSubject(ctx context.Context, providerID, subjectID, callbackID string, result Result) error Start(context.Context, Dest) error Stop(context.Context, Dest) error diff --git a/notification/slack/channel.go b/notification/slack/channel.go index cc7460380c..e507e2789e 100644 --- a/notification/slack/channel.go +++ b/notification/slack/channel.go @@ -29,6 +29,8 @@ type ChannelSender struct { listMx sync.Mutex chanMx sync.Mutex teamMx sync.Mutex + + recv notification.Receiver } const ( @@ -38,6 +40,7 @@ const ( ) var _ notification.Sender = &ChannelSender{} +var _ notification.ReceiverSetter = &ChannelSender{} func NewChannelSender(ctx context.Context, cfg Config) (*ChannelSender, error) { return &ChannelSender{ @@ -48,6 +51,10 @@ func NewChannelSender(ctx context.Context, cfg Config) (*ChannelSender, error) { }, nil } +func (s *ChannelSender) SetReceiver(r notification.Receiver) { + s.recv = r +} + // Channel contains information about a Slack channel. type Channel struct { ID string @@ -234,6 +241,12 @@ func alertLink(ctx context.Context, id int, summary string) string { return fmt.Sprintf("<%s|Alert #%d: %s>", cfg.CallbackURL(path), id, slackutilsx.EscapeMessage(summary)) } +const ( + alertResponseBlockID = "block_alert_response" + alertCloseActionID = "action_alert_close" + alertAckActionID = "action_alert_ack" +) + func alertMsgOption(ctx context.Context, callbackID string, id int, summary, details, logEntry string, status alert.Status) slack.MsgOption { blocks := []slack.Block{ slack.NewSectionBlock( @@ -247,17 +260,17 @@ func alertMsgOption(ctx context.Context, callbackID string, id int, summary, det color = colorAcked actions = []slack.Block{ slack.NewDividerBlock(), - slack.NewActionBlock("alert_response", - slack.NewButtonBlockElement("close", callbackID, slack.NewTextBlockObject("plain_text", "Close", false, false)), + slack.NewActionBlock(alertResponseBlockID, + slack.NewButtonBlockElement(alertCloseActionID, callbackID, slack.NewTextBlockObject("plain_text", "Close", false, false)), ), } case alert.StatusTriggered: color = colorUnacked actions = []slack.Block{ slack.NewDividerBlock(), - slack.NewActionBlock("alert_response", - slack.NewButtonBlockElement("ack", callbackID, slack.NewTextBlockObject("plain_text", "Acknowledge", false, false)), - slack.NewButtonBlockElement("close", callbackID, slack.NewTextBlockObject("plain_text", "Close", false, false)), + slack.NewActionBlock(alertResponseBlockID, + slack.NewButtonBlockElement(alertAckActionID, callbackID, slack.NewTextBlockObject("plain_text", "Acknowledge", false, false)), + slack.NewButtonBlockElement(alertCloseActionID, callbackID, slack.NewTextBlockObject("plain_text", "Close", false, false)), ), } case alert.StatusClosed: diff --git a/notification/slack/servemessageaction.go b/notification/slack/servemessageaction.go new file mode 100644 index 0000000000..980950b617 --- /dev/null +++ b/notification/slack/servemessageaction.go @@ -0,0 +1,58 @@ +package slack + +import ( + "encoding/json" + "net/http" + + "github.com/target/goalert/notification" + "github.com/target/goalert/util/errutil" + "github.com/target/goalert/validation" +) + +func (s *ChannelSender) ServeMessageAction(w http.ResponseWriter, req *http.Request) { + ctx := req.Context() + + var payload struct { + Type string + User struct { + ID string `json:"id"` + TeamID string `json:"team_id"` + } + Actions []struct { + ActionID string `json:"action_id"` + BlockID string `json:"block_id"` + Value string `json:"value"` + } + } + err := json.Unmarshal([]byte(req.FormValue("payload")), &payload) + if errutil.HTTPError(ctx, w, err) { + return + } + + if len(payload.Actions) != 1 { + errutil.HTTPError(ctx, w, validation.NewFieldError("payload", "invalid payload")) + return + } + + act := payload.Actions[0] + if act.BlockID != alertResponseBlockID { + errutil.HTTPError(ctx, w, validation.NewFieldErrorf("block_id", "unknown block ID '%s'", act.BlockID)) + return + } + + var res notification.Result + switch act.ActionID { + case alertAckActionID: + res = notification.ResultAcknowledge + case alertCloseActionID: + res = notification.ResultResolve + default: + errutil.HTTPError(ctx, w, validation.NewFieldErrorf("action_id", "unknown action ID '%s'", act.ActionID)) + return + } + + err = s.recv.ReceiveSubject(ctx, "slack:"+payload.User.TeamID, payload.User.ID, act.Value, res) + if errutil.HTTPError(ctx, w, err) { + return + } +} diff --git a/user/store.go b/user/store.go index 24896663bc..acac21ba85 100644 --- a/user/store.go +++ b/user/store.go @@ -39,6 +39,8 @@ type Store struct { findOneForUpdate *sql.Stmt + findOneBySubject *sql.Stmt + insertUserAuthSubject *sql.Stmt deleteUserAuthSubject *sql.Stmt @@ -120,6 +122,14 @@ func NewStore(ctx context.Context, db *sql.DB) (*Store, error) { updateRotationPart: p.P(`UPDATE rotation_participants SET user_id = $2 WHERE id = $1`), deleteRotationPart: p.P(`DELETE FROM rotation_participants WHERE id = $1`), + findOneBySubject: p.P(` + SELECT + u.id, u.name, u.email, u.avatar_url, u.role, u.alert_status_log_contact_method_id, false + FROM auth_subjects s + JOIN users u ON u.id = s.user_id + WHERE s.provider_id = $1 AND s.subject_id = $2 + `), + findOne: p.P(` SELECT u.id, u.name, u.email, u.avatar_url, u.role, u.alert_status_log_contact_method_id, fav is distinct from null @@ -596,6 +606,33 @@ func (s *Store) FindOneTx(ctx context.Context, tx *sql.Tx, id string, forUpdate return &u, nil } +// FindOneBySubject will find a user matching the subjectID for the given providerID. +func (s *Store) FindOneBySubject(ctx context.Context, providerID, subjectID string) (*User, error) { + err := permission.LimitCheckAny(ctx, permission.Admin) + if err != nil { + return nil, err + } + + err = validate.Many( + validate.SubjectID("ProviderID", providerID), + validate.SubjectID("SubjectID", subjectID), + ) + if err != nil { + return nil, err + } + + row := s.findOneBySubject.QueryRowContext(ctx, providerID, subjectID) + var u User + err = u.scanFrom(row.Scan) + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + if err != nil { + return nil, err + } + return &u, nil +} + // FindSomeAuthSubjectsForProvider returns up to `limit` auth subjects associated with a given providerID. // // afterSubjectID can be specified for paginating responses. Results are sorted by subject id. diff --git a/validation/fieldvalidationerror.go b/validation/fieldvalidationerror.go index dec6a9138f..100ba533bc 100644 --- a/validation/fieldvalidationerror.go +++ b/validation/fieldvalidationerror.go @@ -102,6 +102,11 @@ func NewFieldError(fieldName string, reason string) FieldError { return &fieldError{reason: reason, fieldName: fieldName, stack: errors.New("").(stackTracer).StackTrace()} } +// NewFieldError will create a new FieldError for the given field and reason +func NewFieldErrorf(fieldName string, reason string, args ...interface{}) FieldError { + return &fieldError{reason: fmt.Sprintf(reason, args...), fieldName: fieldName, stack: errors.New("").(stackTracer).StackTrace()} +} + // IsValidationError will determine if an error's cause is a field validation error. func IsValidationError(err error) bool { var e validation From 64dcd801eb61807122b8d2a837dd3024e7bfbd99 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Tue, 2 Nov 2021 17:42:45 -0500 Subject: [PATCH 03/18] add ephemeral response --- notification/slack/servemessageaction.go | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/notification/slack/servemessageaction.go b/notification/slack/servemessageaction.go index 980950b617..ee9ede81df 100644 --- a/notification/slack/servemessageaction.go +++ b/notification/slack/servemessageaction.go @@ -2,10 +2,14 @@ package slack import ( "encoding/json" + "errors" + "fmt" "net/http" + "github.com/slack-go/slack" "github.com/target/goalert/notification" "github.com/target/goalert/util/errutil" + "github.com/target/goalert/util/log" "github.com/target/goalert/validation" ) @@ -13,7 +17,11 @@ func (s *ChannelSender) ServeMessageAction(w http.ResponseWriter, req *http.Requ ctx := req.Context() var payload struct { - Type string + Type string + ResponseURL string `json:"response_url"` + Channel struct { + ID string + } User struct { ID string `json:"id"` TeamID string `json:"team_id"` @@ -52,6 +60,18 @@ func (s *ChannelSender) ServeMessageAction(w http.ResponseWriter, req *http.Requ } err = s.recv.ReceiveSubject(ctx, "slack:"+payload.User.TeamID, payload.User.ID, act.Value, res) + if errors.Is(err, notification.ErrUnknownSubject) { + log.Log(ctx, fmt.Errorf("unknown provider/subject ID for Slack 'slack:%s/%s'", payload.User.TeamID, payload.User.ID)) + err = s.withClient(ctx, func(c *slack.Client) error { + _, err := c.PostEphemeralContext(ctx, payload.Channel.ID, payload.User.ID, + slack.MsgOptionResponseURL(payload.ResponseURL, "ephemeral"), + + // TODO: add user-link/OAUTH flow + slack.MsgOptionText("Your Slack account isn't currently linked to GoAlert, the admin will need to set this up for it to work.", false), + ) + return err + }) + } if errutil.HTTPError(ctx, w, err) { return } From afb8d2549f81ceeefd499ecb33e85d67c6e4cf84 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Tue, 2 Nov 2021 17:51:41 -0500 Subject: [PATCH 04/18] get basic two-way working --- alert/log/store.go | 24 ++++++++++++++---------- engine/sendmessage.go | 11 +++++------ notification/alertstatus.go | 15 ++++++++++++--- notification/slack/channel.go | 15 +++++++-------- 4 files changed, 38 insertions(+), 27 deletions(-) diff --git a/alert/log/store.go b/alert/log/store.go index bdd88e94f2..9d26eab3ed 100644 --- a/alert/log/store.go +++ b/alert/log/store.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/target/goalert/integrationkey" + "github.com/target/goalert/notification" "github.com/target/goalert/notificationchannel" "github.com/target/goalert/permission" "github.com/target/goalert/user/contactmethod" @@ -60,9 +61,10 @@ func NewDB(ctx context.Context, db *sql.DB) (*DB, error) { return &DB{ db: db, lookupCallbackType: p.P(` - select cm."type" + select cm."type", ch."type" from outgoing_messages log - join user_contact_methods cm on cm.id = log.contact_method_id + left join user_contact_methods cm on cm.id = log.contact_method_id + left join notification_channels ch on ch.id = log.channel_id where log.id = $1 `), lookupCMType: p.P(` @@ -341,20 +343,22 @@ func (db *DB) logAny(ctx context.Context, tx *sql.Tx, insertStmt *sql.Stmt, id i case permission.SourceTypeNotificationCallback: r.subject._type = SubjectTypeUser - var cmType contactmethod.Type - err = txWrap(ctx, tx, db.lookupCallbackType).QueryRowContext(ctx, src.ID).Scan(&cmType) + var dt notification.ScannableDestType + err = txWrap(ctx, tx, db.lookupCallbackType).QueryRowContext(ctx, src.ID).Scan(&dt.CM, &dt.NC) if err != nil { - return errors.Wrap(err, "lookup contact method type for callback ID") + return errors.Wrap(err, "lookup notification type for callback ID") } - switch cmType { - case contactmethod.TypeVoice: + switch dt.DestType() { + case notification.DestTypeVoice: r.subject.classifier = "Voice" - case contactmethod.TypeSMS: + case notification.DestTypeSMS: r.subject.classifier = "SMS" - case contactmethod.TypeEmail: + case notification.DestTypeUserEmail: r.subject.classifier = "Email" - case contactmethod.TypeWebhook: + case notification.DestTypeUserWebhook: r.subject.classifier = "Webhook" + case notification.DestTypeSlackChannel: + r.subject.classifier = "Slack" } r.subject.userID.String = permission.UserID(ctx) if r.subject.userID.String != "" { diff --git a/engine/sendmessage.go b/engine/sendmessage.go index acc5a27b82..6d950edcfe 100644 --- a/engine/sendmessage.go +++ b/engine/sendmessage.go @@ -6,7 +6,6 @@ import ( "fmt" "github.com/pkg/errors" - "github.com/target/goalert/alert" alertlog "github.com/target/goalert/alert/log" "github.com/target/goalert/engine/message" "github.com/target/goalert/notification" @@ -104,14 +103,14 @@ func (p *Engine) sendMessage(ctx context.Context, msg *message.Message) (*notifi return nil, fmt.Errorf("could not find original notification for alert %d to %s", msg.AlertID, msg.Dest.String()) } - var status alert.Status + var status notification.AlertState switch e.Type() { case alertlog.TypeAcknowledged: - status = alert.StatusActive + status = notification.AlertStateAcknowledged case alertlog.TypeEscalated: - status = alert.StatusTriggered + status = notification.AlertStateUnacknowledged case alertlog.TypeClosed: - status = alert.StatusClosed + status = notification.AlertStateClosed } notifMsg = notification.AlertStatus{ @@ -121,7 +120,7 @@ func (p *Engine) sendMessage(ctx context.Context, msg *message.Message) (*notifi LogEntry: e.String(), Summary: a.Summary, Details: a.Details, - NewAlertStatus: status, + NewAlertState: status, OriginalStatus: *stat, } case notification.MessageTypeTest: diff --git a/notification/alertstatus.go b/notification/alertstatus.go index bf3b49044f..4793c022e5 100644 --- a/notification/alertstatus.go +++ b/notification/alertstatus.go @@ -1,6 +1,15 @@ package notification -import "github.com/target/goalert/alert" +// AlertState is the current state of an Alert. +type AlertState int + +// All alert states +const ( + AlertStateUnknown AlertState = iota + AlertStateUnacknowledged + AlertStateAcknowledged + AlertStateClosed +) type AlertStatus struct { Dest Dest @@ -16,8 +25,8 @@ type AlertStatus struct { // OriginalStatus is the status of the first Alert notification to this Dest for this AlertID. OriginalStatus SendResult - // NewAlertStatus contains the most recent status for the alert. - NewAlertStatus alert.Status + // NewAlertState contains the most recent state of the alert. + NewAlertState AlertState } var _ Message = &AlertStatus{} diff --git a/notification/slack/channel.go b/notification/slack/channel.go index e507e2789e..77e1041817 100644 --- a/notification/slack/channel.go +++ b/notification/slack/channel.go @@ -9,7 +9,6 @@ import ( "github.com/pkg/errors" "github.com/slack-go/slack" "github.com/slack-go/slack/slackutilsx" - "github.com/target/goalert/alert" "github.com/target/goalert/config" "github.com/target/goalert/notification" "github.com/target/goalert/permission" @@ -247,7 +246,7 @@ const ( alertAckActionID = "action_alert_ack" ) -func alertMsgOption(ctx context.Context, callbackID string, id int, summary, details, logEntry string, status alert.Status) slack.MsgOption { +func alertMsgOption(ctx context.Context, callbackID string, id int, summary, details, logEntry string, state notification.AlertState) slack.MsgOption { blocks := []slack.Block{ slack.NewSectionBlock( slack.NewTextBlockObject("mrkdwn", alertLink(ctx, id, summary), false, false), nil, nil), @@ -255,8 +254,8 @@ func alertMsgOption(ctx context.Context, callbackID string, id int, summary, det var color string var actions []slack.Block - switch status { - case alert.StatusActive: + switch state { + case notification.AlertStateAcknowledged: color = colorAcked actions = []slack.Block{ slack.NewDividerBlock(), @@ -264,7 +263,7 @@ func alertMsgOption(ctx context.Context, callbackID string, id int, summary, det slack.NewButtonBlockElement(alertCloseActionID, callbackID, slack.NewTextBlockObject("plain_text", "Close", false, false)), ), } - case alert.StatusTriggered: + case notification.AlertStateUnacknowledged: color = colorUnacked actions = []slack.Block{ slack.NewDividerBlock(), @@ -273,7 +272,7 @@ func alertMsgOption(ctx context.Context, callbackID string, id int, summary, det slack.NewButtonBlockElement(alertCloseActionID, callbackID, slack.NewTextBlockObject("plain_text", "Close", false, false)), ), } - case alert.StatusClosed: + case notification.AlertStateClosed: color = colorClosed details = "" } @@ -317,11 +316,11 @@ func (s *ChannelSender) Send(ctx context.Context, msg notification.Message) (*no break } - opts = append(opts, alertMsgOption(ctx, t.CallbackID, t.AlertID, t.Summary, t.Details, "Unacknowledged", alert.StatusTriggered)) + opts = append(opts, alertMsgOption(ctx, t.CallbackID, t.AlertID, t.Summary, t.Details, "Unacknowledged", notification.AlertStateUnacknowledged)) case notification.AlertStatus: opts = append(opts, slack.MsgOptionUpdate(t.OriginalStatus.ProviderMessageID.ExternalID), - alertMsgOption(ctx, t.OriginalStatus.ID, t.AlertID, t.Summary, t.Details, t.LogEntry, t.NewAlertStatus), + alertMsgOption(ctx, t.OriginalStatus.ID, t.AlertID, t.Summary, t.Details, t.LogEntry, t.NewAlertState), ) case notification.AlertBundle: opts = append(opts, slack.MsgOptionText( From 24e1af12a57e4c76e01d9a706ebc9e0bb5b126e1 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Wed, 3 Nov 2021 16:07:55 -0500 Subject: [PATCH 05/18] start of action support in mockslack --- devtools/mockslack/actions.go | 102 +++++++++++++++++++++ devtools/mockslack/api.go | 2 + devtools/mockslack/chatpostmessage.go | 101 +++++++++++++++++--- devtools/mockslack/chatpostmessage_test.go | 6 +- devtools/mockslack/server.go | 17 ++++ devtools/mockslack/state.go | 1 + smoketest/harness/harness.go | 1 + smoketest/harness/slack.go | 55 +++++++++++ smoketest/statusupdateschannel_test.go | 14 ++- 9 files changed, 281 insertions(+), 18 deletions(-) create mode 100644 devtools/mockslack/actions.go diff --git a/devtools/mockslack/actions.go b/devtools/mockslack/actions.go new file mode 100644 index 0000000000..4bad265bf1 --- /dev/null +++ b/devtools/mockslack/actions.go @@ -0,0 +1,102 @@ +package mockslack + +import ( + "encoding/json" + "errors" + "fmt" + "net/http" + "net/url" + "strings" +) + +type actionItem struct { + ActionID string `json:"action_id"` + BlockID string `json:"block_id"` + Text struct { + Type string + Text string + } + Value string + Type string +} +type actionBody struct { + Type string + AppID string `json:"api_app_id"` + Channel struct{ ID string } + User struct { + ID string + Username string + Name string + TeamID string `json:"team_id"` + } + ResponseURL string `json:"response_url"` + Actions []actionItem +} + +func (s *Server) ServeActionResponse(w http.ResponseWriter, r *http.Request) { + actData := r.URL.Query().Get("action") + var p actionBody + err := json.Unmarshal([]byte(actData), &p) + if respondErr(w, err) { + return + } + r.Form.Set("channel", p.Channel.ID) + s.ServeChatPostMessage(w, r) +} + +// PerformActionAs will preform the action as the given user. +func (s *Server) PerformActionAs(userID string, a Action) error { + usr := s.user(userID) + if usr == nil { + return errors.New("invalid Slack user ID") + } + + app := s.app(a.AppID) + if app == nil { + return errors.New("invalid Slack app ID") + } + + actionData, err := json.Marshal(a) + if err != nil { + return err + } + + var p actionBody + p.Type = "block_actions" + p.User.ID = usr.ID + p.User.Username = usr.Name + p.User.Name = usr.Name + p.User.TeamID = a.TeamID + p.Channel.ID = a.ChannelID + p.AppID = a.AppID + p.ResponseURL = strings.TrimSuffix(s.urlPrefix, "/") + "/actions/response?action=" + url.QueryEscape(string(actionData)) + + var action actionItem + action.ActionID = a.ActionID + action.BlockID = a.BlockID + action.Text.Type = "plain_text" + action.Text.Text = a.Text + action.Value = a.Value + action.Type = "button" + p.Actions = append(p.Actions, action) + + data, err := json.Marshal(p) + if err != nil { + return fmt.Errorf("serialize action payload: %w", err) + } + + v := make(url.Values) + v.Set("payload", string(data)) + + resp, err := http.PostForm(app.ActionURL, v) + if err != nil { + return fmt.Errorf("perform action: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != 200 { + return fmt.Errorf("perform action: %s", resp.Status) + } + + return nil +} diff --git a/devtools/mockslack/api.go b/devtools/mockslack/api.go index f72ca7dff0..b3e2475c70 100644 --- a/devtools/mockslack/api.go +++ b/devtools/mockslack/api.go @@ -27,4 +27,6 @@ type Message struct { User string `json:"user"` Broadcast bool `json:"reply_broadcast"` Color string `json:"color"` + + Actions []Action } diff --git a/devtools/mockslack/chatpostmessage.go b/devtools/mockslack/chatpostmessage.go index 670df551d0..9832b915e0 100644 --- a/devtools/mockslack/chatpostmessage.go +++ b/devtools/mockslack/chatpostmessage.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "net/http" "strconv" "strings" @@ -16,6 +17,8 @@ type ChatPostMessageOptions struct { Text string Color string + Actions []Action + AsUser bool UpdateTS string @@ -89,6 +92,8 @@ func (st *API) ChatPostMessage(ctx context.Context, opts ChatPostMessageOptions) User: user, Color: opts.Color, + Actions: opts.Actions, + UpdateTS: opts.UpdateTS, ThreadTS: opts.ThreadTS, @@ -101,24 +106,41 @@ func (st *API) ChatPostMessage(ctx context.Context, opts ChatPostMessageOptions) var errNoAttachment = errors.New("no attachment") -func attachmentsText(value string) (text, color string, err error) { +type attachments struct { + Text string + Color string + Actions []Action +} +type Action struct { + ChannelID string + AppID string + TeamID string + + BlockID string + ActionID string + Text string + Value string +} + +func attachmentsText(appID, teamID, chanID, value string) (*attachments, error) { if value == "" { - return "", "", errNoAttachment + return nil, errNoAttachment } - type textBlock struct{ Text string } var data [1]struct { Color string Blocks []struct { - Elements []textBlock + Type string + BlockID string `json:"block_id"` + Elements json.RawMessage Text textBlock } } - - err = json.Unmarshal([]byte(value), &data) + fmt.Println(value) + err := json.Unmarshal([]byte(value), &data) if err != nil { - return "", "", err + return nil, err } var payload strings.Builder @@ -129,14 +151,53 @@ func attachmentsText(value string) (text, color string, err error) { payload.WriteString(b.Text + "\n") } + var actions []Action for _, b := range data[0].Blocks { appendText(b.Text) - for _, e := range b.Elements { - appendText(e) + switch b.Type { + case "context": + var txtEl []textBlock + err = json.Unmarshal(b.Elements, &txtEl) + if err != nil { + return nil, err + } + + for _, e := range txtEl { + appendText(e) + } + case "actions": + var acts []struct { + Text textBlock + ActionID string `json:"action_id"` + Value string + } + err = json.Unmarshal(b.Elements, &acts) + if err != nil { + return nil, err + } + + for _, a := range acts { + actions = append(actions, Action{ + ChannelID: chanID, + TeamID: teamID, + AppID: appID, + BlockID: b.BlockID, + ActionID: a.ActionID, + Text: a.Text.Text, + Value: a.Value, + }) + } + default: + continue } + } - return payload.String(), data[0].Color, nil + return &attachments{ + Text: payload.String(), + Color: data[0].Color, + Actions: actions, + }, nil } // ServeChatPostMessage serves a request to the `chat.postMessage` API call. @@ -156,10 +217,27 @@ func (s *Server) ServeChatUpdate(w http.ResponseWriter, req *http.Request) { func (s *Server) serveChatPostMessage(w http.ResponseWriter, req *http.Request, isUpdate bool) { chanID := req.FormValue("channel") - text, color, err := attachmentsText(req.FormValue("attachments")) + var text, color string + var actions []Action + + var appID string + s.mx.Lock() + for id := range s.apps { + if appID != "" { + panic("multiple apps not supported") + } + appID = id + } + s.mx.Unlock() + + attachment, err := attachmentsText(appID, s.teamID, chanID, req.FormValue("attachments")) if err == errNoAttachment { err = nil text = req.FormValue("text") + } else { + text = attachment.Text + color = attachment.Color + actions = attachment.Actions } if respondErr(w, err) { return @@ -174,6 +252,7 @@ func (s *Server) serveChatPostMessage(w http.ResponseWriter, req *http.Request, ChannelID: chanID, Text: text, Color: color, + Actions: actions, AsUser: req.FormValue("as_user") == "true", ThreadTS: req.FormValue("thread_ts"), UpdateTS: updateTS, diff --git a/devtools/mockslack/chatpostmessage_test.go b/devtools/mockslack/chatpostmessage_test.go index ff35352f94..9bfce1b757 100644 --- a/devtools/mockslack/chatpostmessage_test.go +++ b/devtools/mockslack/chatpostmessage_test.go @@ -9,9 +9,9 @@ import ( func TestAttachmentsText(t *testing.T) { const data = `[{"color":"#862421","blocks":[{"type":"section","text":{"type":"mrkdwn","text":"\u003chttp://127.0.0.1:39999/alerts/1|Alert #1: testing\u003e"}},{"type":"section","text":{"type":"mrkdwn","text":"\u003e "}},{"type":"context","elements":[{"type":"plain_text","text":"Unacknowledged"}]}]}]` - text, color, err := attachmentsText(data) + att, err := attachmentsText("", "", "", data) assert.NoError(t, err) - assert.Equal(t, "#862421", color) - assert.Equal(t, "\n> \nUnacknowledged\n", text) + assert.Equal(t, "#862421", att.Color) + assert.Equal(t, "\n> \nUnacknowledged\n", att.Text) } diff --git a/devtools/mockslack/server.go b/devtools/mockslack/server.go index 83be15ce17..d31e92a401 100644 --- a/devtools/mockslack/server.go +++ b/devtools/mockslack/server.go @@ -16,6 +16,8 @@ type Server struct { mux *http.ServeMux handler http.Handler + + urlPrefix string } // NewServer creates a new blank Server. @@ -25,6 +27,7 @@ func NewServer() *Server { state: newState(), } + srv.mux.HandleFunc("/actions/response", srv.ServeActionResponse) srv.mux.HandleFunc("/api/chat.postMessage", srv.ServeChatPostMessage) srv.mux.HandleFunc("/api/chat.update", srv.ServeChatUpdate) srv.mux.HandleFunc("/api/conversations.info", srv.ServeConversationsInfo) @@ -66,6 +69,11 @@ func NewServer() *Server { return srv } +// SetURLPrefix will update the URL prefix for this server. +func (s *Server) SetURLPrefix(prefix string) { + s.urlPrefix = prefix +} + // TokenCookieName is the name of a cookie containing a token for a user session. const TokenCookieName = "slack_token" @@ -75,6 +83,14 @@ type AppInfo struct { ClientID string ClientSecret string AccessToken string + + ActionURL string +} + +func (s *Server) SetActionURL(appID string, actionURL string) { + s.mx.Lock() + defer s.mx.Unlock() + s.apps[appID].App.ActionURL = actionURL } // InstallApp will "install" a new app to this Slack server using pre-configured AppInfo. @@ -121,6 +137,7 @@ func (st *state) InstallStaticApp(app AppInfo, scopes ...string) (*AppInfo, erro Name: app.Name, Secret: app.ClientSecret, AuthToken: tok, + ActionURL: app.ActionURL, }, } diff --git a/devtools/mockslack/state.go b/devtools/mockslack/state.go index 501d6e302a..ad7cc901f0 100644 --- a/devtools/mockslack/state.go +++ b/devtools/mockslack/state.go @@ -47,6 +47,7 @@ type App struct { Name string Secret string AuthToken *AuthToken + ActionURL string } type channelState struct { diff --git a/smoketest/harness/harness.go b/smoketest/harness/harness.go index 4ea991bbac..b9c96dda31 100644 --- a/smoketest/harness/harness.go +++ b/smoketest/harness/harness.go @@ -317,6 +317,7 @@ func (h *Harness) Start() { h.t.Fatalf("failed to start backend: %v", err) } h.TwilioNumber("") // register default number + h.slack.SetActionURL(h.slackApp.ClientID, h.backend.URL()+"/api/v2/slack/message-action") go h.backend.Run(context.Background()) err = h.backend.WaitForStartup(ctx) diff --git a/smoketest/harness/slack.go b/smoketest/harness/slack.go index 32a9014e56..007a93fbdc 100644 --- a/smoketest/harness/slack.go +++ b/smoketest/harness/slack.go @@ -2,6 +2,7 @@ package harness import ( "net/http/httptest" + "sort" "strings" "time" @@ -21,6 +22,7 @@ type SlackChannel interface { Name() string ExpectMessage(keywords ...string) SlackMessage + // ExpectEphemeralMessage(keywords ...string) SlackMessage } type SlackMessageState interface { @@ -32,6 +34,16 @@ type SlackMessageState interface { // AssertColor asserts that the message has the given color bar value. AssertColor(color string) + + // AssertActions asserts that the message includes the given action buttons. + AssertActions(labels ...string) + + // Action returns the action with the given label. + Action(label string) SlackAction +} + +type SlackAction interface { + Click() } type SlackMessage interface { @@ -67,6 +79,48 @@ type slackMessage struct { mockslack.Message } +type slackAction struct { + *slackMessage + mockslack.Action +} + +func (msg *slackMessage) AssertActions(text ...string) { + msg.h.t.Helper() + + require.Equalf(msg.h.t, len(text), len(msg.Actions), "message actions") + sort.Slice(text, func(i, j int) bool { return text[i] < text[j] }) + sort.Slice(msg.Actions, func(i, j int) bool { return msg.Actions[i].Text < msg.Actions[j].Text }) + for i, a := range msg.Actions { + require.Equalf(msg.h.t, text[i], a.Text, "message action text") + } +} +func (msg *slackMessage) Action(text string) SlackAction { + msg.h.t.Helper() + + var a *mockslack.Action + for _, action := range msg.Actions { + if action.Text != text { + continue + } + a = &action + break + } + require.NotNil(msg.h.t, a, "could not find action with that text") + + return &slackAction{ + slackMessage: msg, + Action: *a, + } +} + +func (a *slackAction) Click() { + a.h.t.Helper() + + a.h.t.Logf("clicking action: %s", a.Text) + err := a.h.slack.PerformActionAs(a.h.slackUser.ID, a.Action) + require.NoError(a.h.t, err, "perform Slack action") +} + func (h *Harness) Slack() SlackServer { return h.slack } func (s *slackServer) WaitAndAssert() { @@ -231,4 +285,5 @@ func (h *Harness) initSlack() { h.slackApp = h.slack.InstallApp("GoAlert Smoketest", "bot") h.slackUser = h.slack.NewUser("GoAlert Smoketest User") + h.slack.SetURLPrefix(h.slackS.URL) } diff --git a/smoketest/statusupdateschannel_test.go b/smoketest/statusupdateschannel_test.go index 55bb3a29af..2393678ba4 100644 --- a/smoketest/statusupdateschannel_test.go +++ b/smoketest/statusupdateschannel_test.go @@ -36,25 +36,31 @@ func TestStatusUpdatesChannel(t *testing.T) { a := h.CreateAlertWithDetails(h.UUID("sid"), "testing", "details") msg := h.Slack().Channel("test").ExpectMessage("testing", "details") msg.AssertColor("#862421") - - a.Ack() + msg.AssertActions("Acknowledge", "Close") + msg.Action("Acknowledge").Click() + t.FailNow() updated := msg.ExpectUpdate() updated.AssertText("Ack", "testing", "details") updated.AssertColor("#867321") + updated.AssertActions("Close") a.Escalate() updated = msg.ExpectUpdate() updated.AssertText("Escalated", "testing", "details") updated.AssertColor("#862421") - + updated.AssertActions("Acknowledge", "Close") msg.ExpectBroadcastReply("testing") - a.Close() + updated.Action("Close").Click() updated = msg.ExpectUpdate() updated.AssertText("Closed", "testing") updated.AssertNotText("details") updated.AssertColor("#218626") + + updated.AssertActions() // no actions + + t.Fail() } From 60d14ee98ee5d8df5c2e87e61f52e7e4ecfb4c47 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Wed, 3 Nov 2021 16:10:12 -0500 Subject: [PATCH 06/18] fix test --- smoketest/statusupdateschannel_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/smoketest/statusupdateschannel_test.go b/smoketest/statusupdateschannel_test.go index 2393678ba4..584d9f770f 100644 --- a/smoketest/statusupdateschannel_test.go +++ b/smoketest/statusupdateschannel_test.go @@ -37,8 +37,7 @@ func TestStatusUpdatesChannel(t *testing.T) { msg := h.Slack().Channel("test").ExpectMessage("testing", "details") msg.AssertColor("#862421") msg.AssertActions("Acknowledge", "Close") - msg.Action("Acknowledge").Click() - t.FailNow() + a.Ack() updated := msg.ExpectUpdate() updated.AssertText("Ack", "testing", "details") @@ -53,7 +52,7 @@ func TestStatusUpdatesChannel(t *testing.T) { updated.AssertActions("Acknowledge", "Close") msg.ExpectBroadcastReply("testing") - updated.Action("Close").Click() + a.Close() updated = msg.ExpectUpdate() updated.AssertText("Closed", "testing") @@ -62,5 +61,4 @@ func TestStatusUpdatesChannel(t *testing.T) { updated.AssertActions() // no actions - t.Fail() } From 3c4a38b3e49069d53dca218a4013eb73d602862b Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Thu, 4 Nov 2021 10:30:33 -0500 Subject: [PATCH 07/18] opt-in to interactive messages --- config/config.go | 2 ++ graphql2/mapconfig.go | 7 +++++++ notification/slack/channel.go | 3 ++- web/src/schema.d.ts | 1 + 4 files changed, 12 insertions(+), 1 deletion(-) diff --git a/config/config.go b/config/config.go index fed49f4102..26a5b9a147 100644 --- a/config/config.go +++ b/config/config.go @@ -87,6 +87,8 @@ type Config struct { // The `xoxb-` prefix is documented by Slack. // https://api.slack.com/docs/token-types#bot AccessToken string `password:"true" info:"Slack app bot user OAuth access token (should start with xoxb-)."` + + InteractiveMessages bool `info:"Enable interactive messages (e.g. buttons)."` } Twilio struct { diff --git a/graphql2/mapconfig.go b/graphql2/mapconfig.go index ebd8716e5a..fab4aa37c9 100644 --- a/graphql2/mapconfig.go +++ b/graphql2/mapconfig.go @@ -62,6 +62,7 @@ func MapConfigValues(cfg config.Config) []ConfigValue { {ID: "Slack.ClientID", Type: ConfigTypeString, Description: "", Value: cfg.Slack.ClientID}, {ID: "Slack.ClientSecret", Type: ConfigTypeString, Description: "", Value: cfg.Slack.ClientSecret, Password: true}, {ID: "Slack.AccessToken", Type: ConfigTypeString, Description: "Slack app bot user OAuth access token (should start with xoxb-).", Value: cfg.Slack.AccessToken, Password: true}, + {ID: "Slack.InteractiveMessages", Type: ConfigTypeBoolean, Description: "Enable interactive messages (e.g. buttons).", Value: fmt.Sprintf("%t", cfg.Slack.InteractiveMessages)}, {ID: "Twilio.Enable", Type: ConfigTypeBoolean, Description: "Enables sending and processing of Voice and SMS messages through the Twilio notification provider.", Value: fmt.Sprintf("%t", cfg.Twilio.Enable)}, {ID: "Twilio.AccountSID", Type: ConfigTypeString, Description: "", Value: cfg.Twilio.AccountSID}, {ID: "Twilio.AuthToken", Type: ConfigTypeString, Description: "The primary Auth Token for Twilio. Must be primary (not secondary) for request valiation.", Value: cfg.Twilio.AuthToken, Password: true}, @@ -277,6 +278,12 @@ func ApplyConfigValues(cfg config.Config, vals []ConfigValueInput) (config.Confi cfg.Slack.ClientSecret = v.Value case "Slack.AccessToken": cfg.Slack.AccessToken = v.Value + case "Slack.InteractiveMessages": + val, err := parseBool(v.ID, v.Value) + if err != nil { + return cfg, err + } + cfg.Slack.InteractiveMessages = val case "Twilio.Enable": val, err := parseBool(v.ID, v.Value) if err != nil { diff --git a/notification/slack/channel.go b/notification/slack/channel.go index 77e1041817..134eab73e5 100644 --- a/notification/slack/channel.go +++ b/notification/slack/channel.go @@ -285,7 +285,8 @@ func alertMsgOption(ctx context.Context, callbackID string, id int, summary, det blocks = append(blocks, slack.NewContextBlock("", slack.NewTextBlockObject("plain_text", logEntry, false, false)), ) - if len(actions) > 0 { + cfg := config.FromContext(ctx) + if len(actions) > 0 && cfg.Slack.InteractiveMessages { blocks = append(blocks, actions...) } diff --git a/web/src/schema.d.ts b/web/src/schema.d.ts index 1157ff8e78..0bdecbfa87 100644 --- a/web/src/schema.d.ts +++ b/web/src/schema.d.ts @@ -977,6 +977,7 @@ type ConfigID = | 'Slack.ClientID' | 'Slack.ClientSecret' | 'Slack.AccessToken' + | 'Slack.InteractiveMessages' | 'Twilio.Enable' | 'Twilio.AccountSID' | 'Twilio.AuthToken' From 13a07b7f9ddb4ae9d0ab4be764986c9eea39d25c Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Thu, 4 Nov 2021 11:52:13 -0500 Subject: [PATCH 08/18] add interaction test --- devtools/mockslack/actions.go | 50 ++++++++++++++++-- devtools/mockslack/api.go | 3 ++ devtools/mockslack/chatpostmessage.go | 7 +++ devtools/mockslack/server.go | 3 ++ smoketest/harness/slack.go | 19 ++++++- smoketest/slackinteraction_test.go | 73 +++++++++++++++++++++++++++ 6 files changed, 149 insertions(+), 6 deletions(-) create mode 100644 smoketest/slackinteraction_test.go diff --git a/devtools/mockslack/actions.go b/devtools/mockslack/actions.go index 4bad265bf1..9a9e9195b1 100644 --- a/devtools/mockslack/actions.go +++ b/devtools/mockslack/actions.go @@ -34,14 +34,48 @@ type actionBody struct { } func (s *Server) ServeActionResponse(w http.ResponseWriter, r *http.Request) { + var req struct { + Text string + Type string `json:"response_type"` + } + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + if req.Type != "ephemeral" { + http.Error(w, "unexpected response type", http.StatusBadRequest) + return + } + actData := r.URL.Query().Get("action") - var p actionBody - err := json.Unmarshal([]byte(actData), &p) + var a Action + err := json.Unmarshal([]byte(actData), &a) if respondErr(w, err) { return } - r.Form.Set("channel", p.Channel.ID) - s.ServeChatPostMessage(w, r) + + msg, err := s.API().ChatPostMessage(r.Context(), ChatPostMessageOptions{ + ChannelID: a.ChannelID, + Text: req.Text, + User: r.URL.Query().Get("user"), + }) + if respondErr(w, err) { + return + } + + var respData struct { + response + TS string + Channel string `json:"channel"` + Message *Message `json:"message"` + } + respData.TS = msg.TS + respData.OK = true + respData.Channel = msg.ChannelID + respData.Message = msg + + respondWith(w, respData) } // PerformActionAs will preform the action as the given user. @@ -69,7 +103,13 @@ func (s *Server) PerformActionAs(userID string, a Action) error { p.User.TeamID = a.TeamID p.Channel.ID = a.ChannelID p.AppID = a.AppID - p.ResponseURL = strings.TrimSuffix(s.urlPrefix, "/") + "/actions/response?action=" + url.QueryEscape(string(actionData)) + + tok := s.newToken(AuthToken{ + User: userID, + + Scopes: []string{"bot"}, + }) + p.ResponseURL = fmt.Sprintf("%s/actions/response?token=%s&user=%s&action=%s", strings.TrimSuffix(s.urlPrefix, "/"), url.QueryEscape(tok.ID), url.QueryEscape(usr.ID), url.QueryEscape(string(actionData))) var action actionItem action.ActionID = a.ActionID diff --git a/devtools/mockslack/api.go b/devtools/mockslack/api.go index b3e2475c70..ffdce64b3c 100644 --- a/devtools/mockslack/api.go +++ b/devtools/mockslack/api.go @@ -28,5 +28,8 @@ type Message struct { Broadcast bool `json:"reply_broadcast"` Color string `json:"color"` + ChannelID string + ToUserID string + Actions []Action } diff --git a/devtools/mockslack/chatpostmessage.go b/devtools/mockslack/chatpostmessage.go index 9832b915e0..31b6bcd262 100644 --- a/devtools/mockslack/chatpostmessage.go +++ b/devtools/mockslack/chatpostmessage.go @@ -21,6 +21,8 @@ type ChatPostMessageOptions struct { AsUser bool + User string + UpdateTS string ThreadTS string Broadcast bool @@ -92,6 +94,9 @@ func (st *API) ChatPostMessage(ctx context.Context, opts ChatPostMessageOptions) User: user, Color: opts.Color, + ChannelID: opts.ChannelID, + ToUserID: opts.User, + Actions: opts.Actions, UpdateTS: opts.UpdateTS, @@ -257,6 +262,8 @@ func (s *Server) serveChatPostMessage(w http.ResponseWriter, req *http.Request, ThreadTS: req.FormValue("thread_ts"), UpdateTS: updateTS, + User: req.FormValue("user"), + Broadcast: req.FormValue("reply_broadcast") == "true", }) if respondErr(w, err) { diff --git a/devtools/mockslack/server.go b/devtools/mockslack/server.go index d31e92a401..52015a2f41 100644 --- a/devtools/mockslack/server.go +++ b/devtools/mockslack/server.go @@ -29,6 +29,7 @@ func NewServer() *Server { srv.mux.HandleFunc("/actions/response", srv.ServeActionResponse) srv.mux.HandleFunc("/api/chat.postMessage", srv.ServeChatPostMessage) + srv.mux.HandleFunc("/api/chat.postEphemeral", srv.ServeChatPostMessage) srv.mux.HandleFunc("/api/chat.update", srv.ServeChatUpdate) srv.mux.HandleFunc("/api/conversations.info", srv.ServeConversationsInfo) srv.mux.HandleFunc("/api/conversations.list", srv.ServeConversationsList) @@ -83,6 +84,7 @@ type AppInfo struct { ClientID string ClientSecret string AccessToken string + TeamID string ActionURL string } @@ -107,6 +109,7 @@ func (st *state) InstallStaticApp(app AppInfo, scopes ...string) (*AppInfo, erro if app.AccessToken == "" { app.AccessToken = st.gen.UserAccessToken() } + app.TeamID = st.teamID if !clientIDRx.MatchString(app.ClientID) { return nil, errors.Errorf("invalid client ID format: %s", app.ClientID) diff --git a/smoketest/harness/slack.go b/smoketest/harness/slack.go index 007a93fbdc..0c40999fa4 100644 --- a/smoketest/harness/slack.go +++ b/smoketest/harness/slack.go @@ -1,6 +1,7 @@ package harness import ( + "context" "net/http/httptest" "sort" "strings" @@ -22,7 +23,7 @@ type SlackChannel interface { Name() string ExpectMessage(keywords ...string) SlackMessage - // ExpectEphemeralMessage(keywords ...string) SlackMessage + ExpectEphemeralMessage(keywords ...string) SlackMessage } type SlackMessageState interface { @@ -170,6 +171,14 @@ func (ch *slackChannel) ExpectMessage(keywords ...string) SlackMessage { }, keywords...) } +func (ch *slackChannel) ExpectEphemeralMessage(keywords ...string) SlackMessage { + ch.h.t.Helper() + return ch.expectMessageFunc("ephemeral", func(msg mockslack.Message) bool { + // only return non-thread replies + return msg.ToUserID != "" + }, keywords...) +} + func (ch *slackChannel) expectMessageFunc(desc string, test func(mockslack.Message) bool, keywords ...string) *slackMessage { ch.h.t.Helper() @@ -285,5 +294,13 @@ func (h *Harness) initSlack() { h.slackApp = h.slack.InstallApp("GoAlert Smoketest", "bot") h.slackUser = h.slack.NewUser("GoAlert Smoketest User") + h.slack.SetURLPrefix(h.slackS.URL) } + +// LinkSlackUser creates a link between the GraphQL user and the smoketest Slack user. +func (h *Harness) LinkSlackUser() { + _, err := h.db.Exec(context.Background(), `insert into auth_subjects (provider_id, subject_id, user_id) values ($1, $2, $3)`, + "slack:"+h.slackApp.TeamID, h.slackUser.ID, DefaultGraphQLAdminUserID) + require.NoError(h.t, err, "insert Slack auth subject") +} diff --git a/smoketest/slackinteraction_test.go b/smoketest/slackinteraction_test.go new file mode 100644 index 0000000000..1fa6c2a020 --- /dev/null +++ b/smoketest/slackinteraction_test.go @@ -0,0 +1,73 @@ +package smoketest + +import ( + "testing" + + "github.com/target/goalert/smoketest/harness" +) + +// TestSlackInteraction checks that interactive slack messages work properly. +func TestSlackInteraction(t *testing.T) { + t.Parallel() + + sql := ` + insert into escalation_policies (id, name) + values + ({{uuid "eid"}}, 'esc policy'); + insert into escalation_policy_steps (id, escalation_policy_id) + values + ({{uuid "esid"}}, {{uuid "eid"}}); + + insert into notification_channels (id, type, name, value) + values + ({{uuid "chan"}}, 'SLACK', '#test', {{slackChannelID "test"}}); + + insert into escalation_policy_actions (escalation_policy_step_id, channel_id) + values + ({{uuid "esid"}}, {{uuid "chan"}}); + + insert into services (id, escalation_policy_id, name) + values + ({{uuid "sid"}}, {{uuid "eid"}}, 'service'); +` + h := harness.NewHarness(t, sql, "slack-user-link") + defer h.Close() + + h.SetConfigValue("Slack.InteractiveMessages", "true") + + a := h.CreateAlertWithDetails(h.UUID("sid"), "testing", "details") + + ch := h.Slack().Channel("test") + msg := ch.ExpectMessage("testing", "details") + msg.AssertColor("#862421") + msg.AssertActions("Acknowledge", "Close") + + h.IgnoreErrorsWith("unknown provider/subject") + msg.Action("Acknowledge").Click() // expect ephemeral + ch.ExpectEphemeralMessage("GoAlert", "admin") + + h.LinkSlackUser() + msg.Action("Acknowledge").Click() + + updated := msg.ExpectUpdate() + updated.AssertText("Ack", "testing", "details") + updated.AssertColor("#867321") + updated.AssertActions("Close") + + a.Escalate() + + updated = msg.ExpectUpdate() + updated.AssertText("Escalated", "testing", "details") + updated.AssertColor("#862421") + updated.AssertActions("Acknowledge", "Close") + msg.ExpectBroadcastReply("testing") + + msg.Action("Close").Click() + + updated = msg.ExpectUpdate() + updated.AssertText("Closed", "testing") + updated.AssertNotText("details") + updated.AssertColor("#218626") + + updated.AssertActions() // no actions +} From cb2eb21806582e4c245408cc647aa1ea4010c4bb Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Thu, 4 Nov 2021 11:54:18 -0500 Subject: [PATCH 09/18] no actions without interactivity enabled --- smoketest/statusupdateschannel_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/smoketest/statusupdateschannel_test.go b/smoketest/statusupdateschannel_test.go index 584d9f770f..da22500a32 100644 --- a/smoketest/statusupdateschannel_test.go +++ b/smoketest/statusupdateschannel_test.go @@ -36,20 +36,20 @@ func TestStatusUpdatesChannel(t *testing.T) { a := h.CreateAlertWithDetails(h.UUID("sid"), "testing", "details") msg := h.Slack().Channel("test").ExpectMessage("testing", "details") msg.AssertColor("#862421") - msg.AssertActions("Acknowledge", "Close") + msg.AssertActions() a.Ack() updated := msg.ExpectUpdate() updated.AssertText("Ack", "testing", "details") updated.AssertColor("#867321") - updated.AssertActions("Close") + updated.AssertActions() a.Escalate() updated = msg.ExpectUpdate() updated.AssertText("Escalated", "testing", "details") updated.AssertColor("#862421") - updated.AssertActions("Acknowledge", "Close") + updated.AssertActions() msg.ExpectBroadcastReply("testing") a.Close() From f1e7a847b16ece89b8ee9cd7ee26b52ee67b4491 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Thu, 4 Nov 2021 11:58:59 -0500 Subject: [PATCH 10/18] remove debug line --- devtools/mockslack/chatpostmessage.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/devtools/mockslack/chatpostmessage.go b/devtools/mockslack/chatpostmessage.go index 31b6bcd262..8782803780 100644 --- a/devtools/mockslack/chatpostmessage.go +++ b/devtools/mockslack/chatpostmessage.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "errors" - "fmt" "net/http" "strconv" "strings" @@ -142,7 +141,6 @@ func attachmentsText(appID, teamID, chanID, value string) (*attachments, error) Text textBlock } } - fmt.Println(value) err := json.Unmarshal([]byte(value), &data) if err != nil { return nil, err From ba8359c5139e55ae27c8e4b94c77a8d07d8591f9 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Thu, 4 Nov 2021 15:10:30 -0500 Subject: [PATCH 11/18] add signature validation for slack requests --- config/config.go | 7 ++- devtools/mockslack/actions.go | 20 ++++++++- devtools/mockslack/server.go | 4 ++ devtools/mockslack/state.go | 2 + graphql2/mapconfig.go | 3 ++ notification/slack/servemessageaction.go | 54 +++++++++++++++++++++++- smoketest/harness/harness.go | 1 + smoketest/harness/slack.go | 8 +++- 8 files changed, 95 insertions(+), 4 deletions(-) diff --git a/config/config.go b/config/config.go index 26a5b9a147..77e82ace5b 100644 --- a/config/config.go +++ b/config/config.go @@ -88,7 +88,8 @@ type Config struct { // https://api.slack.com/docs/token-types#bot AccessToken string `password:"true" info:"Slack app bot user OAuth access token (should start with xoxb-)."` - InteractiveMessages bool `info:"Enable interactive messages (e.g. buttons)."` + SigningSecret string `password:"true" info:"Signing secret to verify requests from slack."` + InteractiveMessages bool `info:"Enable interactive messages (e.g. buttons)."` } Twilio struct { @@ -396,6 +397,7 @@ func (cfg Config) Validate() error { validatePath("OIDC.UserInfoEmailPath", cfg.OIDC.UserInfoEmailPath), validatePath("OIDC.UserInfoEmailVerifiedPath", cfg.OIDC.UserInfoEmailVerifiedPath), validatePath("OIDC.UserInfoNamePath", cfg.OIDC.UserInfoNamePath), + validateKey("Slack.SigningSecret", cfg.Slack.SigningSecret), ) if cfg.OIDC.IssuerURL != "" { @@ -419,6 +421,9 @@ func (cfg Config) Validate() error { if cfg.SMTP.From != "" { err = validate.Many(err, validate.Email("SMTP.From", cfg.SMTP.From)) } + if cfg.Slack.InteractiveMessages && cfg.Slack.SigningSecret == "" { + err = validate.Many(err, validation.NewFieldError("Slack.SigningSecret", "required to enable Slack interactive messages")) + } err = validate.Many( err, diff --git a/devtools/mockslack/actions.go b/devtools/mockslack/actions.go index 9a9e9195b1..717c3ed31c 100644 --- a/devtools/mockslack/actions.go +++ b/devtools/mockslack/actions.go @@ -1,12 +1,17 @@ package mockslack import ( + "bytes" + "crypto/hmac" + "crypto/sha256" "encoding/json" "errors" "fmt" "net/http" "net/url" + "strconv" "strings" + "time" ) type actionItem struct { @@ -127,8 +132,21 @@ func (s *Server) PerformActionAs(userID string, a Action) error { v := make(url.Values) v.Set("payload", string(data)) + data = []byte(v.Encode()) - resp, err := http.PostForm(app.ActionURL, v) + req, err := http.NewRequest("POST", app.ActionURL, bytes.NewReader(data)) + if err != nil { + return fmt.Errorf("create action request: %w", err) + } + + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + t := time.Now() + req.Header.Set("X-Slack-Request-Timestamp", strconv.FormatInt(t.Unix(), 10)) + h := hmac.New(sha256.New, []byte(app.SigningSecret)) + fmt.Fprintf(h, "v0:%d:%s", t.Unix(), string(data)) + req.Header.Set("X-Slack-Signature", "v0="+fmt.Sprintf("%x", h.Sum(nil))) + + resp, err := http.DefaultClient.Do(req) if err != nil { return fmt.Errorf("perform action: %w", err) } diff --git a/devtools/mockslack/server.go b/devtools/mockslack/server.go index 52015a2f41..a2b3e842a5 100644 --- a/devtools/mockslack/server.go +++ b/devtools/mockslack/server.go @@ -86,6 +86,8 @@ type AppInfo struct { AccessToken string TeamID string + SigningSecret string + ActionURL string } @@ -141,6 +143,8 @@ func (st *state) InstallStaticApp(app AppInfo, scopes ...string) (*AppInfo, erro Secret: app.ClientSecret, AuthToken: tok, ActionURL: app.ActionURL, + + SigningSecret: app.SigningSecret, }, } diff --git a/devtools/mockslack/state.go b/devtools/mockslack/state.go index ad7cc901f0..b9d98b24b5 100644 --- a/devtools/mockslack/state.go +++ b/devtools/mockslack/state.go @@ -48,6 +48,8 @@ type App struct { Secret string AuthToken *AuthToken ActionURL string + + SigningSecret string } type channelState struct { diff --git a/graphql2/mapconfig.go b/graphql2/mapconfig.go index fab4aa37c9..74c472c932 100644 --- a/graphql2/mapconfig.go +++ b/graphql2/mapconfig.go @@ -62,6 +62,7 @@ func MapConfigValues(cfg config.Config) []ConfigValue { {ID: "Slack.ClientID", Type: ConfigTypeString, Description: "", Value: cfg.Slack.ClientID}, {ID: "Slack.ClientSecret", Type: ConfigTypeString, Description: "", Value: cfg.Slack.ClientSecret, Password: true}, {ID: "Slack.AccessToken", Type: ConfigTypeString, Description: "Slack app bot user OAuth access token (should start with xoxb-).", Value: cfg.Slack.AccessToken, Password: true}, + {ID: "Slack.SigningSecret", Type: ConfigTypeString, Description: "Signing secret to verify requests from slack.", Value: cfg.Slack.SigningSecret, Password: true}, {ID: "Slack.InteractiveMessages", Type: ConfigTypeBoolean, Description: "Enable interactive messages (e.g. buttons).", Value: fmt.Sprintf("%t", cfg.Slack.InteractiveMessages)}, {ID: "Twilio.Enable", Type: ConfigTypeBoolean, Description: "Enables sending and processing of Voice and SMS messages through the Twilio notification provider.", Value: fmt.Sprintf("%t", cfg.Twilio.Enable)}, {ID: "Twilio.AccountSID", Type: ConfigTypeString, Description: "", Value: cfg.Twilio.AccountSID}, @@ -278,6 +279,8 @@ func ApplyConfigValues(cfg config.Config, vals []ConfigValueInput) (config.Confi cfg.Slack.ClientSecret = v.Value case "Slack.AccessToken": cfg.Slack.AccessToken = v.Value + case "Slack.SigningSecret": + cfg.Slack.SigningSecret = v.Value case "Slack.InteractiveMessages": val, err := parseBool(v.ID, v.Value) if err != nil { diff --git a/notification/slack/servemessageaction.go b/notification/slack/servemessageaction.go index ee9ede81df..f705918a52 100644 --- a/notification/slack/servemessageaction.go +++ b/notification/slack/servemessageaction.go @@ -1,12 +1,19 @@ package slack import ( + "crypto/hmac" + "crypto/sha256" + "encoding/hex" "encoding/json" "errors" "fmt" + "io" "net/http" + "strconv" + "time" "github.com/slack-go/slack" + "github.com/target/goalert/config" "github.com/target/goalert/notification" "github.com/target/goalert/util/errutil" "github.com/target/goalert/util/log" @@ -15,6 +22,51 @@ import ( func (s *ChannelSender) ServeMessageAction(w http.ResponseWriter, req *http.Request) { ctx := req.Context() + cfg := config.FromContext(ctx) + + if !cfg.Slack.InteractiveMessages { + http.Error(w, "not enabled", http.StatusNotFound) + return + } + + var newBody struct { + io.Reader + io.Closer + } + + h := hmac.New(sha256.New, []byte(cfg.Slack.SigningSecret)) + tsStr := req.Header.Get("X-Slack-Request-Timestamp") + io.WriteString(h, "v0:"+tsStr+":") + newBody.Reader = io.TeeReader(req.Body, h) + newBody.Closer = req.Body + req.Body = newBody + + err := req.ParseForm() + if err != nil { + log.Log(ctx, err) + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + + ts, err := strconv.ParseInt(tsStr, 10, 64) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + diff := time.Since(time.Unix(ts, 0)) + if diff < 0 { + diff = -diff + } + if diff > 5*time.Minute { + http.Error(w, "timestamp too old", http.StatusBadRequest) + return + } + + sig := "v0=" + hex.EncodeToString(h.Sum(nil)) + if hmac.Equal([]byte(req.Header.Get("X-Slack-Signature")), []byte(sig)) { + http.Error(w, "invalid signature", http.StatusBadRequest) + return + } var payload struct { Type string @@ -32,7 +84,7 @@ func (s *ChannelSender) ServeMessageAction(w http.ResponseWriter, req *http.Requ Value string `json:"value"` } } - err := json.Unmarshal([]byte(req.FormValue("payload")), &payload) + err = json.Unmarshal([]byte(req.FormValue("payload")), &payload) if errutil.HTTPError(ctx, w, err) { return } diff --git a/smoketest/harness/harness.go b/smoketest/harness/harness.go index b9c96dda31..b3e89c409e 100644 --- a/smoketest/harness/harness.go +++ b/smoketest/harness/harness.go @@ -244,6 +244,7 @@ func (h *Harness) Start() { cfg.Slack.AccessToken = h.slackApp.AccessToken cfg.Slack.ClientID = h.slackApp.ClientID cfg.Slack.ClientSecret = h.slackApp.ClientSecret + cfg.Slack.SigningSecret = SlackTestSigningSecret cfg.Twilio.Enable = true cfg.Twilio.AccountSID = twilioAccountSID cfg.Twilio.AuthToken = twilioAuthToken diff --git a/smoketest/harness/slack.go b/smoketest/harness/slack.go index 0c40999fa4..b1de737990 100644 --- a/smoketest/harness/slack.go +++ b/smoketest/harness/slack.go @@ -12,6 +12,10 @@ import ( "github.com/target/goalert/devtools/mockslack" ) +const ( + SlackTestSigningSecret = "secret" +) + type SlackServer interface { Channel(string) SlackChannel @@ -292,7 +296,9 @@ func (h *Harness) initSlack() { } h.slackS = httptest.NewServer(h.slack) - h.slackApp = h.slack.InstallApp("GoAlert Smoketest", "bot") + app, err := h.slack.InstallStaticApp(mockslack.AppInfo{Name: "GoAlert Smoketest", SigningSecret: SlackTestSigningSecret}, "bot") + require.NoError(h.t, err) + h.slackApp = *app h.slackUser = h.slack.NewUser("GoAlert Smoketest User") h.slack.SetURLPrefix(h.slackS.URL) From 93d7a170dc63a20a61f5e38e02125d02d9a8b899 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Tue, 9 Nov 2021 08:38:49 -0600 Subject: [PATCH 12/18] ignore external ID on update --- notification/slack/channel.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/notification/slack/channel.go b/notification/slack/channel.go index 134eab73e5..467455e6c9 100644 --- a/notification/slack/channel.go +++ b/notification/slack/channel.go @@ -305,6 +305,7 @@ func (s *ChannelSender) Send(ctx context.Context, msg notification.Message) (*no // Note: We don't use cfg.ApplicationName() here since that is configured in the Slack app as the bot name. var opts []slack.MsgOption + var isUpdate bool switch t := msg.(type) { case notification.Alert: if t.OriginalStatus != nil { @@ -319,6 +320,7 @@ func (s *ChannelSender) Send(ctx context.Context, msg notification.Message) (*no opts = append(opts, alertMsgOption(ctx, t.CallbackID, t.AlertID, t.Summary, t.Details, "Unacknowledged", notification.AlertStateUnacknowledged)) case notification.AlertStatus: + isUpdate = true opts = append(opts, slack.MsgOptionUpdate(t.OriginalStatus.ProviderMessageID.ExternalID), alertMsgOption(ctx, t.OriginalStatus.ID, t.AlertID, t.Summary, t.Details, t.LogEntry, t.NewAlertState), @@ -346,6 +348,10 @@ func (s *ChannelSender) Send(ctx context.Context, msg notification.Message) (*no return nil, err } + if isUpdate { + msgTS = "" + } + return ¬ification.SentMessage{ ExternalID: msgTS, State: notification.StateDelivered, From ea0bd5eb36275e845cbaddf9c560add5ed939f28 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Tue, 9 Nov 2021 08:44:08 -0600 Subject: [PATCH 13/18] regen schema --- web/src/schema.d.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/web/src/schema.d.ts b/web/src/schema.d.ts index 0bdecbfa87..b140dfdbcb 100644 --- a/web/src/schema.d.ts +++ b/web/src/schema.d.ts @@ -977,6 +977,7 @@ type ConfigID = | 'Slack.ClientID' | 'Slack.ClientSecret' | 'Slack.AccessToken' + | 'Slack.SigningSecret' | 'Slack.InteractiveMessages' | 'Twilio.Enable' | 'Twilio.AccountSID' From eb5705a5f813d7edd2bff1552f361d99d5371073 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Tue, 9 Nov 2021 08:44:18 -0600 Subject: [PATCH 14/18] ignore duplicate request errors --- notification/slack/servemessageaction.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/notification/slack/servemessageaction.go b/notification/slack/servemessageaction.go index f705918a52..504cb00fbb 100644 --- a/notification/slack/servemessageaction.go +++ b/notification/slack/servemessageaction.go @@ -13,6 +13,7 @@ import ( "time" "github.com/slack-go/slack" + "github.com/target/goalert/alert" "github.com/target/goalert/config" "github.com/target/goalert/notification" "github.com/target/goalert/util/errutil" @@ -124,6 +125,10 @@ func (s *ChannelSender) ServeMessageAction(w http.ResponseWriter, req *http.Requ return err }) } + if alert.IsAlreadyAcknowledged(err) || alert.IsAlreadyClosed(err) { + // ignore errors from duplicate requests + return + } if errutil.HTTPError(ctx, w, err) { return } From cbe2bf4d2c985752de26c69d8aec059f5375b43c Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Tue, 9 Nov 2021 10:48:46 -0600 Subject: [PATCH 15/18] use more descriptive name --- devtools/mockslack/chatpostmessage.go | 5 +++-- devtools/mockslack/chatpostmessage_test.go | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/devtools/mockslack/chatpostmessage.go b/devtools/mockslack/chatpostmessage.go index 8782803780..971924dca6 100644 --- a/devtools/mockslack/chatpostmessage.go +++ b/devtools/mockslack/chatpostmessage.go @@ -126,7 +126,8 @@ type Action struct { Value string } -func attachmentsText(appID, teamID, chanID, value string) (*attachments, error) { +// parseAttachments parses the attachments from the payload value. +func parseAttachments(appID, teamID, chanID, value string) (*attachments, error) { if value == "" { return nil, errNoAttachment } @@ -233,7 +234,7 @@ func (s *Server) serveChatPostMessage(w http.ResponseWriter, req *http.Request, } s.mx.Unlock() - attachment, err := attachmentsText(appID, s.teamID, chanID, req.FormValue("attachments")) + attachment, err := parseAttachments(appID, s.teamID, chanID, req.FormValue("attachments")) if err == errNoAttachment { err = nil text = req.FormValue("text") diff --git a/devtools/mockslack/chatpostmessage_test.go b/devtools/mockslack/chatpostmessage_test.go index 9bfce1b757..8ef61b4ce8 100644 --- a/devtools/mockslack/chatpostmessage_test.go +++ b/devtools/mockslack/chatpostmessage_test.go @@ -6,10 +6,10 @@ import ( "github.com/stretchr/testify/assert" ) -func TestAttachmentsText(t *testing.T) { +func TestParseAttachments(t *testing.T) { const data = `[{"color":"#862421","blocks":[{"type":"section","text":{"type":"mrkdwn","text":"\u003chttp://127.0.0.1:39999/alerts/1|Alert #1: testing\u003e"}},{"type":"section","text":{"type":"mrkdwn","text":"\u003e "}},{"type":"context","elements":[{"type":"plain_text","text":"Unacknowledged"}]}]}]` - att, err := attachmentsText("", "", "", data) + att, err := parseAttachments("", "", "", data) assert.NoError(t, err) assert.Equal(t, "#862421", att.Color) assert.Equal(t, "\n> \nUnacknowledged\n", att.Text) From 68cee7e69b487f534832fdacd2846215d7d71734 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Tue, 9 Nov 2021 10:48:59 -0600 Subject: [PATCH 16/18] break out validation to helper func --- notification/slack/servemessageaction.go | 38 ++++++++++++++---------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/notification/slack/servemessageaction.go b/notification/slack/servemessageaction.go index 504cb00fbb..bb4d1a408a 100644 --- a/notification/slack/servemessageaction.go +++ b/notification/slack/servemessageaction.go @@ -21,14 +21,8 @@ import ( "github.com/target/goalert/validation" ) -func (s *ChannelSender) ServeMessageAction(w http.ResponseWriter, req *http.Request) { - ctx := req.Context() - cfg := config.FromContext(ctx) - - if !cfg.Slack.InteractiveMessages { - http.Error(w, "not enabled", http.StatusNotFound) - return - } +func validateRequestSignature(req *http.Request) error { + cfg := config.FromContext(req.Context()) var newBody struct { io.Reader @@ -44,31 +38,43 @@ func (s *ChannelSender) ServeMessageAction(w http.ResponseWriter, req *http.Requ err := req.ParseForm() if err != nil { - log.Log(ctx, err) - http.Error(w, err.Error(), http.StatusBadRequest) - return + return fmt.Errorf("failed to parse form: %w", err) } ts, err := strconv.ParseInt(tsStr, 10, 64) if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) - return + return fmt.Errorf("failed to parse timestamp: %w", err) } diff := time.Since(time.Unix(ts, 0)) if diff < 0 { diff = -diff } if diff > 5*time.Minute { - http.Error(w, "timestamp too old", http.StatusBadRequest) - return + return fmt.Errorf("timestamp too old: %s", diff) } sig := "v0=" + hex.EncodeToString(h.Sum(nil)) if hmac.Equal([]byte(req.Header.Get("X-Slack-Signature")), []byte(sig)) { - http.Error(w, "invalid signature", http.StatusBadRequest) + return fmt.Errorf("invalid signature") + } + + return nil +} + +func (s *ChannelSender) ServeMessageAction(w http.ResponseWriter, req *http.Request) { + ctx := req.Context() + cfg := config.FromContext(ctx) + + if !cfg.Slack.InteractiveMessages { + http.Error(w, "not enabled", http.StatusNotFound) return } + err := validateRequestSignature(req) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + } + var payload struct { Type string ResponseURL string `json:"response_url"` From 681de2ee368fc8b40ba9714374263cc2e542e926 Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Tue, 9 Nov 2021 10:49:11 -0600 Subject: [PATCH 17/18] simplify NewFieldErrorf --- validation/fieldvalidationerror.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/validation/fieldvalidationerror.go b/validation/fieldvalidationerror.go index 100ba533bc..b3e279431b 100644 --- a/validation/fieldvalidationerror.go +++ b/validation/fieldvalidationerror.go @@ -103,8 +103,8 @@ func NewFieldError(fieldName string, reason string) FieldError { } // NewFieldError will create a new FieldError for the given field and reason -func NewFieldErrorf(fieldName string, reason string, args ...interface{}) FieldError { - return &fieldError{reason: fmt.Sprintf(reason, args...), fieldName: fieldName, stack: errors.New("").(stackTracer).StackTrace()} +func NewFieldErrorf(fieldName string, reasonFormat string, args ...interface{}) FieldError { + return NewFieldError(fieldName, fmt.Sprintf(reasonFormat, args...)) } // IsValidationError will determine if an error's cause is a field validation error. From fc397753fd4b5b6dc0fb979b8bfe978f0c018b1f Mon Sep 17 00:00:00 2001 From: Nathaniel Caza Date: Tue, 9 Nov 2021 10:52:40 -0600 Subject: [PATCH 18/18] add comment for `alertMsgOption` helper --- notification/slack/channel.go | 1 + 1 file changed, 1 insertion(+) diff --git a/notification/slack/channel.go b/notification/slack/channel.go index 467455e6c9..34ecd251c0 100644 --- a/notification/slack/channel.go +++ b/notification/slack/channel.go @@ -246,6 +246,7 @@ const ( alertAckActionID = "action_alert_ack" ) +// alertMsgOption will return the slack.MsgOption for an alert-type message (e.g., notification or status update). func alertMsgOption(ctx context.Context, callbackID string, id int, summary, details, logEntry string, state notification.AlertState) slack.MsgOption { blocks := []slack.Block{ slack.NewSectionBlock(