diff --git a/pkg/services/alerting/notifiers/slack.go b/pkg/services/alerting/notifiers/slack.go index 72a7b7a2c4e..7e68966ca55 100644 --- a/pkg/services/alerting/notifiers/slack.go +++ b/pkg/services/alerting/notifiers/slack.go @@ -13,7 +13,6 @@ import ( "net/url" "os" "path/filepath" - "regexp" "strings" "time" @@ -36,7 +35,7 @@ func init() { Label: "Recipient", Element: alerting.ElementTypeInput, InputType: alerting.InputTypeText, - Description: "Specify channel or user, use #channel-name, @username (has to be all lowercase, no whitespace), or user/channel Slack ID - required unless you provide a webhook", + Description: "Specify channel, private group, or IM channel (can be an encoded ID or a name) - required unless you provide a webhook", PropertyName: "recipient", }, // Logically, this field should be required when not using a webhook, since the Slack API needs a token. @@ -119,8 +118,6 @@ func init() { }) } -var reRecipient *regexp.Regexp = regexp.MustCompile("^((@[a-z0-9][a-zA-Z0-9._-]*)|(#[^ .A-Z]{1,79})|([a-zA-Z0-9]+))$") - const slackAPIEndpoint = "https://slack.com/api/chat.postMessage" // NewSlackNotifier is the constructor for the Slack notifier. @@ -135,11 +132,7 @@ func NewSlackNotifier(model *models.AlertNotification, fn alerting.GetDecryptedV } recipient := strings.TrimSpace(model.Settings.Get("recipient").MustString()) - if recipient != "" { - if !reRecipient.MatchString(recipient) { - return nil, alerting.ValidationError{Reason: fmt.Sprintf("recipient on invalid format: %q", recipient)} - } - } else if apiURL.String() == slackAPIEndpoint { + if recipient == "" && apiURL.String() == slackAPIEndpoint { return nil, alerting.ValidationError{ Reason: "recipient must be specified when using the Slack chat API", } @@ -383,19 +376,21 @@ func (sn *SlackNotifier) sendRequest(ctx context.Context, data []byte) error { } if resp.StatusCode >= 200 && resp.StatusCode < 300 { - // Slack responds to some requests with a JSON document, that might contain an error + // Slack responds to some requests with a JSON document, that might contain an error. rslt := struct { Ok bool `json:"ok"` Err string `json:"error"` }{} - if err := json.Unmarshal(body, &rslt); err != nil { - sn.log.Warn("Failed to unmarshal Slack API response", "url", sn.url.String(), "statusCode", resp.Status, + + // Marshaling can fail if Slack's response body is plain text (e.g. "ok"). + if err := json.Unmarshal(body, &rslt); err != nil && json.Valid(body) { + sn.log.Error("Failed to unmarshal Slack API response", "url", sn.url.String(), "statusCode", resp.Status, "err", err) return fmt.Errorf("failed to unmarshal Slack API response with status code %d: %s", resp.StatusCode, err) } if !rslt.Ok && rslt.Err != "" { - sn.log.Warn("Sending Slack API request failed", "url", sn.url.String(), "statusCode", resp.Status, + sn.log.Error("Sending Slack API request failed", "url", sn.url.String(), "statusCode", resp.Status, "err", rslt.Err) return fmt.Errorf("failed to make Slack API request: %s", rslt.Err) } @@ -405,7 +400,7 @@ func (sn *SlackNotifier) sendRequest(ctx context.Context, data []byte) error { return nil } - sn.log.Warn("Slack API request failed", "url", sn.url.String(), "statusCode", resp.Status, "body", string(body)) + sn.log.Error("Slack API request failed", "url", sn.url.String(), "statusCode", resp.Status, "body", string(body)) return fmt.Errorf("request to Slack API failed with status code %d", resp.StatusCode) } diff --git a/pkg/services/alerting/notifiers/slack_test.go b/pkg/services/alerting/notifiers/slack_test.go index d0a15199bbd..75d30c32dc0 100644 --- a/pkg/services/alerting/notifiers/slack_test.go +++ b/pkg/services/alerting/notifiers/slack_test.go @@ -147,63 +147,6 @@ func TestSlackNotifier(t *testing.T) { assert.Equal(t, "xenc-XXXXXXXX-XXXXXXXX-XXXXXXXXXX", slackNotifier.token) }) - t.Run("with channel recipient with spaces should return an error", func(t *testing.T) { - json := ` - { - "url": "http://google.com", - "recipient": "#open tsdb" - }` - - settingsJSON, err := simplejson.NewJson([]byte(json)) - require.NoError(t, err) - model := &models.AlertNotification{ - Name: "ops", - Type: "slack", - Settings: settingsJSON, - } - - _, err = NewSlackNotifier(model, ossencryption.ProvideService().GetDecryptedValue) - assert.EqualError(t, err, "alert validation error: recipient on invalid format: \"#open tsdb\"") - }) - - t.Run("with user recipient with spaces should return an error", func(t *testing.T) { - json := ` - { - "url": "http://google.com", - "recipient": "@user name" - }` - - settingsJSON, err := simplejson.NewJson([]byte(json)) - require.NoError(t, err) - model := &models.AlertNotification{ - Name: "ops", - Type: "slack", - Settings: settingsJSON, - } - - _, err = NewSlackNotifier(model, ossencryption.ProvideService().GetDecryptedValue) - assert.EqualError(t, err, "alert validation error: recipient on invalid format: \"@user name\"") - }) - - t.Run("with user recipient with uppercase letters should return an error", func(t *testing.T) { - json := ` - { - "url": "http://google.com", - "recipient": "@User" - }` - - settingsJSON, err := simplejson.NewJson([]byte(json)) - require.NoError(t, err) - model := &models.AlertNotification{ - Name: "ops", - Type: "slack", - Settings: settingsJSON, - } - - _, err = NewSlackNotifier(model, ossencryption.ProvideService().GetDecryptedValue) - assert.EqualError(t, err, "alert validation error: recipient on invalid format: \"@User\"") - }) - t.Run("with Slack ID for recipient should work", func(t *testing.T) { json := ` { @@ -273,9 +216,8 @@ func TestSendSlackRequest(t *testing.T) { expectError: false, }, { - name: "No response body", - statusCode: http.StatusOK, - expectError: true, + name: "No response body", + statusCode: http.StatusOK, }, { name: "Success case, unexpected response body", diff --git a/pkg/services/ngalert/notifier/available_channels.go b/pkg/services/ngalert/notifier/available_channels.go index a3c23d87e98..dbbf8328dd5 100644 --- a/pkg/services/ngalert/notifier/available_channels.go +++ b/pkg/services/ngalert/notifier/available_channels.go @@ -380,7 +380,7 @@ func GetAvailableNotifiers() []*alerting.NotifierPlugin { Label: "Recipient", Element: alerting.ElementTypeInput, InputType: alerting.InputTypeText, - Description: "Specify channel or user, use #channel-name, @username (has to be all lowercase, no whitespace), or user/channel Slack ID - required unless you provide a webhook", + Description: "Specify channel, private group, or IM channel (can be an encoded ID or a name) - required unless you provide a webhook", PropertyName: "recipient", }, // Logically, this field should be required when not using a webhook, since the Slack API needs a token. diff --git a/pkg/services/ngalert/notifier/channels/slack.go b/pkg/services/ngalert/notifier/channels/slack.go index 04e9fb60247..7d9af7eb525 100644 --- a/pkg/services/ngalert/notifier/channels/slack.go +++ b/pkg/services/ngalert/notifier/channels/slack.go @@ -10,7 +10,6 @@ import ( "net" "net/http" "net/url" - "regexp" "strings" "time" @@ -42,8 +41,6 @@ type SlackNotifier struct { Token string } -var reRecipient *regexp.Regexp = regexp.MustCompile("^((@[a-z0-9][a-zA-Z0-9._-]*)|(#[^ .A-Z]{1,79})|([a-zA-Z0-9]+))$") - var SlackAPIEndpoint = "https://slack.com/api/chat.postMessage" // NewSlackNotifier is the constructor for the Slack notifier @@ -62,11 +59,7 @@ func NewSlackNotifier(model *NotificationChannelConfig, t *template.Template, fn } recipient := strings.TrimSpace(model.Settings.Get("recipient").MustString()) - if recipient != "" { - if !reRecipient.MatchString(recipient) { - return nil, receiverInitError{Cfg: *model, Reason: fmt.Sprintf("recipient on invalid format: %q", recipient)} - } - } else if apiURL.String() == SlackAPIEndpoint { + if recipient == "" && apiURL.String() == SlackAPIEndpoint { return nil, receiverInitError{Cfg: *model, Reason: "recipient must be specified when using the Slack chat API", } @@ -219,23 +212,25 @@ var sendSlackRequest = func(request *http.Request, logger log.Logger) error { } if resp.StatusCode < 200 || resp.StatusCode >= 300 { - logger.Warn("Slack API request failed", "url", request.URL.String(), "statusCode", resp.Status, "body", string(body)) + logger.Error("Slack API request failed", "url", request.URL.String(), "statusCode", resp.Status, "body", string(body)) return fmt.Errorf("request to Slack API failed with status code %d", resp.StatusCode) } - // Slack responds to some requests with a JSON document, that might contain an error + // Slack responds to some requests with a JSON document, that might contain an error. rslt := struct { Ok bool `json:"ok"` Err string `json:"error"` }{} - if err := json.Unmarshal(body, &rslt); err != nil { - logger.Warn("Failed to unmarshal Slack API response", "url", request.URL.String(), "statusCode", resp.Status, + + // Marshaling can fail if Slack's response body is plain text (e.g. "ok"). + if err := json.Unmarshal(body, &rslt); err != nil && json.Valid(body) { + logger.Error("Failed to unmarshal Slack API response", "url", request.URL.String(), "statusCode", resp.Status, "body", string(body)) return fmt.Errorf("failed to unmarshal Slack API response: %s", err) } if !rslt.Ok && rslt.Err != "" { - logger.Warn("Sending Slack API request failed", "url", request.URL.String(), "statusCode", resp.Status, + logger.Error("Sending Slack API request failed", "url", request.URL.String(), "statusCode", resp.Status, "err", rslt.Err) return fmt.Errorf("failed to make Slack API request: %s", rslt.Err) } diff --git a/pkg/services/ngalert/notifier/channels/slack_test.go b/pkg/services/ngalert/notifier/channels/slack_test.go index 2f137e40e8b..8cd75d24867 100644 --- a/pkg/services/ngalert/notifier/channels/slack_test.go +++ b/pkg/services/ngalert/notifier/channels/slack_test.go @@ -270,9 +270,8 @@ func TestSendSlackRequest(t *testing.T) { expectError: false, }, { - name: "No response body", - statusCode: http.StatusOK, - expectError: true, + name: "No response body", + statusCode: http.StatusOK, }, { name: "Success case, unexpected response body", diff --git a/pkg/services/sqlstore/migrations/ualert/ualert_test.go b/pkg/services/sqlstore/migrations/ualert/ualert_test.go index 3b9dd0ea165..7d6ddbc1ef9 100644 --- a/pkg/services/sqlstore/migrations/ualert/ualert_test.go +++ b/pkg/services/sqlstore/migrations/ualert/ualert_test.go @@ -1,7 +1,6 @@ package ualert import ( - "errors" "fmt" "testing" @@ -31,17 +30,16 @@ func Test_validateAlertmanagerConfig(t *testing.T) { err: fmt.Errorf("failed to validate receiver \"SlackWithBadURL\" of type \"slack\": invalid URL %q: parse %q: net/url: invalid control character in URL", invalidUri, invalidUri), }, { - name: "when a slack receiver has an invalid recipient - it should error", + name: "when a slack receiver has an invalid recipient - it should not error", receivers: []*PostableGrafanaReceiver{ { UID: util.GenerateShortUID(), Name: "SlackWithBadRecipient", Type: "slack", - Settings: simplejson.NewFromAny(map[string]interface{}{"recipient": "this-doesnt-pass"}), + Settings: simplejson.NewFromAny(map[string]interface{}{"recipient": "this passes"}), SecureSettings: map[string]string{"url": "http://webhook.slack.com/myuser"}, }, }, - err: errors.New("failed to validate receiver \"SlackWithBadRecipient\" of type \"slack\": recipient on invalid format: \"this-doesnt-pass\""), }, { name: "when the configuration is valid - it should not error", diff --git a/pkg/tests/api/alerting/api_available_channel_test.go b/pkg/tests/api/alerting/api_available_channel_test.go index 767d1a1b315..fef0f0f15c8 100644 --- a/pkg/tests/api/alerting/api_available_channel_test.go +++ b/pkg/tests/api/alerting/api_available_channel_test.go @@ -790,7 +790,7 @@ var expAvailableChannelJsonOutput = ` "element": "input", "inputType": "text", "label": "Recipient", - "description": "Specify channel or user, use #channel-name, @username (has to be all lowercase, no whitespace), or user/channel Slack ID - required unless you provide a webhook", + "description": "Specify channel, private group, or IM channel (can be an encoded ID or a name) - required unless you provide a webhook", "placeholder": "", "propertyName": "recipient", "selectOptions": null,