Fix issues with Slack contact points (#40953)

* recipient validation regex modified, validation at creation/modification implemented

* Remove validation for recipient, fix tests

* Log level changed from Warn to Error

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 recipient == "" && apiURL.String() == slackAPIEndpoint {
-		if !reRecipient.MatchString(recipient) {
-			return nil, alerting.ValidationError{Reason: fmt.Sprintf("recipient on invalid format: %q", recipient)}
-		}
-	} else if 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)
 }
 

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",
+			name:       "No response body",
-			statusCode:  http.StatusOK,
+			statusCode: http.StatusOK,
-			expectError: true,
 		},
 		{
 			name:          "Success case, unexpected response body",

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.

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 recipient == "" && apiURL.String() == SlackAPIEndpoint {
-		if !reRecipient.MatchString(recipient) {
-			return nil, receiverInitError{Cfg: *model, Reason: fmt.Sprintf("recipient on invalid format: %q", recipient)}
-		}
-	} else if 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)
 	}

pkg/services/ngalert/notifier/channels/slack_test.go

@@ -270,9 +270,8 @@ func TestSendSlackRequest(t *testing.T) {
 			expectError: false,
 		},
 		{
-			name:        "No response body",
+			name:       "No response body",
-			statusCode:  http.StatusOK,
+			statusCode: http.StatusOK,
-			expectError: true,
 		},
 		{
 			name:          "Success case, unexpected response body",

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

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,