From b3c2ecd9b9209413e7272b2fcd7bd3d04f2f85f4 Mon Sep 17 00:00:00 2001 From: Pradeep Murugesan Date: Wed, 25 Jul 2018 14:31:41 +0200 Subject: added the custom icon and username for the outgoing webhook and its response (#9141) * 8272 added the username and icon as part of the model and persisted the same * 8272 added the custome icon and name when set to the web hook response * 8272 changed the infinte loop to timeout after 5 seconds * 8272 fixed review comments --- api4/webhook_test.go | 2 +- app/webhook.go | 6 ++ app/webhook_test.go | 181 +++++++++++++++++++++++++++++++++++++++ i18n/en.json | 7 ++ model/outgoing_webhook.go | 10 +++ model/outgoing_webhook_test.go | 20 +++++ store/sqlstore/upgrade.go | 3 +- store/sqlstore/webhook_store.go | 2 + store/storetest/webhook_store.go | 7 ++ 9 files changed, 236 insertions(+), 2 deletions(-) diff --git a/api4/webhook_test.go b/api4/webhook_test.go index 441fb8bb7..764f25709 100644 --- a/api4/webhook_test.go +++ b/api4/webhook_test.go @@ -256,7 +256,7 @@ func TestCreateOutgoingWebhook(t *testing.T) { th.AddPermissionToRole(model.PERMISSION_MANAGE_WEBHOOKS.Id, model.TEAM_ADMIN_ROLE_ID) th.RemovePermissionFromRole(model.PERMISSION_MANAGE_WEBHOOKS.Id, model.TEAM_USER_ROLE_ID) - hook := &model.OutgoingWebhook{ChannelId: th.BasicChannel.Id, TeamId: th.BasicChannel.TeamId, CallbackURLs: []string{"http://nowhere.com"}} + hook := &model.OutgoingWebhook{ChannelId: th.BasicChannel.Id, TeamId: th.BasicChannel.TeamId, CallbackURLs: []string{"http://nowhere.com"}, Username: "some-user-name", IconURL: "http://some-icon-url/"} rhook, resp := th.SystemAdminClient.CreateOutgoingWebhook(hook) CheckNoError(t, resp) diff --git a/app/webhook.go b/app/webhook.go index 8926c94a8..e801b0467 100644 --- a/app/webhook.go +++ b/app/webhook.go @@ -133,7 +133,13 @@ func (a *App) TriggerWebhook(payload *model.OutgoingWebhookPayload, hook *model. if len(webhookResp.Attachments) > 0 { webhookResp.Props["attachments"] = webhookResp.Attachments } + if a.Config().ServiceSettings.EnablePostUsernameOverride && hook.Username != "" && webhookResp.Username == "" { + webhookResp.Username = hook.Username + } + if a.Config().ServiceSettings.EnablePostIconOverride && hook.IconURL != "" && webhookResp.IconURL == "" { + webhookResp.IconURL = hook.IconURL + } if _, err := a.CreateWebhookPost(hook.CreatorId, channel, text, webhookResp.Username, webhookResp.IconURL, webhookResp.Props, webhookResp.Type, postRootId); err != nil { mlog.Error(fmt.Sprintf("Failed to create response post, err=%v", err)) } diff --git a/app/webhook_test.go b/app/webhook_test.go index 8931100ac..85c52b144 100644 --- a/app/webhook_test.go +++ b/app/webhook_test.go @@ -11,6 +11,9 @@ import ( "github.com/stretchr/testify/require" "github.com/mattermost/mattermost-server/model" + "net/http" + "net/http/httptest" + "time" ) func TestCreateIncomingWebhookForChannel(t *testing.T) { @@ -470,3 +473,181 @@ func TestSplitWebhookPost(t *testing.T) { }) } } + +func TestCreateOutGoingWebhookWithUsernameAndIconURL(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + outgoingWebhook := model.OutgoingWebhook{ + ChannelId: th.BasicChannel.Id, + TeamId: th.BasicChannel.TeamId, + CallbackURLs: []string{"http://nowhere.com"}, + Username: "some-user-name", + IconURL: "http://some-icon/", + DisplayName: "some-display-name", + Description: "some-description", + CreatorId: th.BasicUser.Id, + } + + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableOutgoingWebhooks = true }) + + createdHook, err := th.App.CreateOutgoingWebhook(&outgoingWebhook) + + if err != nil { + t.Fatalf("should not have failed: %v", err.Error()) + } + + assert.NotNil(t, createdHook, "should not be null") + + assert.Equal(t, createdHook.ChannelId, outgoingWebhook.ChannelId) + assert.Equal(t, createdHook.TeamId, outgoingWebhook.TeamId) + assert.Equal(t, createdHook.CallbackURLs, outgoingWebhook.CallbackURLs) + assert.Equal(t, createdHook.Username, outgoingWebhook.Username) + assert.Equal(t, createdHook.IconURL, outgoingWebhook.IconURL) + assert.Equal(t, createdHook.DisplayName, outgoingWebhook.DisplayName) + assert.Equal(t, createdHook.Description, outgoingWebhook.Description) + +} + +func TestTriggerOutGoingWebhookWithUsernameAndIconURL(t *testing.T) { + + getPayload := func(hook *model.OutgoingWebhook, th *TestHelper, channel *model.Channel) *model.OutgoingWebhookPayload { + return &model.OutgoingWebhookPayload{ + Token: hook.Token, + TeamId: hook.TeamId, + TeamDomain: th.BasicTeam.Name, + ChannelId: channel.Id, + ChannelName: channel.Name, + Timestamp: th.BasicPost.CreateAt, + UserId: th.BasicPost.UserId, + UserName: th.BasicUser.Username, + PostId: th.BasicPost.Id, + Text: th.BasicPost.Message, + TriggerWord: "Abracadabra", + FileIds: strings.Join(th.BasicPost.FileIds, ","), + } + } + + waitUntilWebhookResposeIsCreatedAsPost := func(channel *model.Channel, th *TestHelper, t *testing.T, createdPost chan *model.Post) { + go func() { + for i := 0; i < 5; i++ { + time.Sleep(time.Second) + posts, _ := th.App.GetPosts(channel.Id, 0, 5) + if len(posts.Posts) > 0 { + for _, post := range posts.Posts { + createdPost <- post + return + } + } + } + }() + } + + type TestCaseOutgoing struct { + EnablePostUsernameOverride bool + EnablePostIconOverride bool + ExpectedUsername string + ExpectedIconUrl string + WebhookResponse *model.OutgoingWebhookResponse + } + + createOutgoingWebhook := func(channel *model.Channel, testCallBackUrl string, th *TestHelper) (*model.OutgoingWebhook, *model.AppError) { + outgoingWebhook := model.OutgoingWebhook{ + ChannelId: channel.Id, + TeamId: channel.TeamId, + CallbackURLs: []string{testCallBackUrl}, + Username: "some-user-name", + IconURL: "http://some-icon/", + DisplayName: "some-display-name", + Description: "some-description", + CreatorId: th.BasicUser.Id, + TriggerWords: []string{"Abracadabra"}, + ContentType: "application/json", + } + + return th.App.CreateOutgoingWebhook(&outgoingWebhook) + } + + getTestCases := func() map[string]TestCaseOutgoing { + + webHookResponse := "sample response text from test server" + testCasesOutgoing := map[string]TestCaseOutgoing{ + + "Should override username and Icon": { + EnablePostUsernameOverride: true, + EnablePostIconOverride: true, + ExpectedUsername: "some-user-name", + ExpectedIconUrl: "http://some-icon/", + }, + "Should not override username and Icon": { + EnablePostUsernameOverride: false, + EnablePostIconOverride: false, + }, + "Should not override username and Icon if the webhook response already has it": { + EnablePostUsernameOverride: true, + EnablePostIconOverride: true, + ExpectedUsername: "webhookuser", + ExpectedIconUrl: "http://webhok/icon", + WebhookResponse: &model.OutgoingWebhookResponse{Text: &webHookResponse, Username: "webhookuser", IconURL: "http://webhok/icon"}, + }, + } + return testCasesOutgoing + } + + th := Setup().InitBasic() + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.AllowedUntrustedInternalConnections = "localhost 127.0.0.1" + }) + createdPost := make(chan *model.Post) + + for name, testCase := range getTestCases() { + t.Run(name, func(t *testing.T) { + + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.ServiceSettings.EnableOutgoingWebhooks = true + cfg.ServiceSettings.EnablePostUsernameOverride = testCase.EnablePostUsernameOverride + cfg.ServiceSettings.EnablePostIconOverride = testCase.EnablePostIconOverride + }) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if testCase.WebhookResponse != nil { + w.Write([]byte(testCase.WebhookResponse.ToJson())) + } else { + w.Write([]byte(`{"text": "sample response text from test server"}`)) + } + })) + defer ts.Close() + + channel := th.CreateChannel(th.BasicTeam) + hook, _ := createOutgoingWebhook(channel, ts.URL, th) + payload := getPayload(hook, th, channel) + + th.App.TriggerWebhook(payload, hook, th.BasicPost, channel) + + waitUntilWebhookResposeIsCreatedAsPost(channel, th, t, createdPost) + + select { + case webhookPost := <-createdPost: + assert.Equal(t, webhookPost.Message, "sample response text from test server") + assert.Equal(t, webhookPost.Props["from_webhook"], "true") + if testCase.ExpectedIconUrl != "" { + assert.Equal(t, webhookPost.Props["override_icon_url"], testCase.ExpectedIconUrl) + } else { + assert.Nil(t, webhookPost.Props["override_icon_url"]) + } + + if testCase.ExpectedUsername != "" { + assert.Equal(t, webhookPost.Props["override_username"], testCase.ExpectedUsername) + } else { + assert.Nil(t, webhookPost.Props["override_username"]) + } + case <-time.After(5 * time.Second): + t.Fatal("Timeout, webhook response not created as post") + } + + }) + } + +} diff --git a/i18n/en.json b/i18n/en.json index fabe02ff6..629bebc8b 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -4454,6 +4454,13 @@ "id": "model.outgoing_hook.is_valid.words.app_error", "translation": "Invalid trigger words" }, + { + "id": "model.outgoing_hook.username.app_error", + "translation": "Invalid username" + },{ + "id": "model.outgoing_hook.icon_url.app_error", + "translation": "Invalid icon" + }, { "id": "model.plugin_command.error.app_error", "translation": "An error occurred while trying to execute this command." diff --git a/model/outgoing_webhook.go b/model/outgoing_webhook.go index b5dbf34d9..698a226e3 100644 --- a/model/outgoing_webhook.go +++ b/model/outgoing_webhook.go @@ -28,6 +28,8 @@ type OutgoingWebhook struct { DisplayName string `json:"display_name"` Description string `json:"description"` ContentType string `json:"content_type"` + Username string `json:"username"` + IconURL string `json:"icon_url"` } type OutgoingWebhookPayload struct { @@ -181,6 +183,14 @@ func (o *OutgoingWebhook) IsValid() *AppError { return NewAppError("OutgoingWebhook.IsValid", "model.outgoing_hook.is_valid.content_type.app_error", nil, "", http.StatusBadRequest) } + if len(o.Username) > 64 { + return NewAppError("OutgoingWebhook.IsValid", "model.outgoing_hook.username.app_error", nil, "", http.StatusBadRequest) + } + + if len(o.IconURL) > 1024 { + return NewAppError("OutgoingWebhook.IsValid", "model.outgoing_hook.icon_url.app_error", nil, "", http.StatusBadRequest) + } + return nil } diff --git a/model/outgoing_webhook_test.go b/model/outgoing_webhook_test.go index 49441cfea..3241e649f 100644 --- a/model/outgoing_webhook_test.go +++ b/model/outgoing_webhook_test.go @@ -121,6 +121,26 @@ func TestOutgoingWebhookIsValid(t *testing.T) { if err := o.IsValid(); err != nil { t.Fatal(err) } + + o.Username = strings.Repeat("1", 65) + if err := o.IsValid(); err == nil { + t.Fatal("should be invalid") + } + + o.Username = strings.Repeat("1", 64) + if err := o.IsValid(); err != nil { + t.Fatal("should be invalid") + } + + o.IconURL = strings.Repeat("1", 1025) + if err := o.IsValid(); err == nil { + t.Fatal(err) + } + + o.IconURL = strings.Repeat("1", 1024) + if err := o.IsValid(); err != nil { + t.Fatal(err) + } } func TestOutgoingWebhookPayloadToFormValues(t *testing.T) { diff --git a/store/sqlstore/upgrade.go b/store/sqlstore/upgrade.go index 8ea44371c..b2299b223 100644 --- a/store/sqlstore/upgrade.go +++ b/store/sqlstore/upgrade.go @@ -476,7 +476,8 @@ func UpgradeDatabaseToVersion51(sqlStore SqlStore) { func UpgradeDatabaseToVersion52(sqlStore SqlStore) { // TODO: Uncomment following condition when version 5.2.0 is released // if shouldPerformUpgrade(sqlStore, VERSION_5_1_0, VERSION_5_2_0) { - + sqlStore.CreateColumnIfNotExists("OutgoingWebhooks", "Username", "varchar(64)", "varchar(64)", "") + sqlStore.CreateColumnIfNotExists("OutgoingWebhooks", "IconURL", "varchar(1024)", "varchar(1024)", "") // saveSchemaVersion(sqlStore, VERSION_5_2_0) // } } diff --git a/store/sqlstore/webhook_store.go b/store/sqlstore/webhook_store.go index 45ad90e52..f3c572aaf 100644 --- a/store/sqlstore/webhook_store.go +++ b/store/sqlstore/webhook_store.go @@ -61,6 +61,8 @@ func NewSqlWebhookStore(sqlStore SqlStore, metrics einterfaces.MetricsInterface) tableo.ColMap("Description").SetMaxSize(128) tableo.ColMap("ContentType").SetMaxSize(128) tableo.ColMap("TriggerWhen").SetMaxSize(1) + tableo.ColMap("Username").SetMaxSize(64) + tableo.ColMap("IconURL").SetMaxSize(1024) } return s diff --git a/store/storetest/webhook_store.go b/store/storetest/webhook_store.go index 7f87ec29d..2dfa2ae53 100644 --- a/store/storetest/webhook_store.go +++ b/store/storetest/webhook_store.go @@ -239,6 +239,8 @@ func testWebhookStoreSaveOutgoing(t *testing.T, ss store.Store) { o1.CreatorId = model.NewId() o1.TeamId = model.NewId() o1.CallbackURLs = []string{"http://nowhere.com/"} + o1.Username = "test-user-name" + o1.IconURL = "http://nowhere.com/icon" if err := (<-ss.Webhook().SaveOutgoing(&o1)).Err; err != nil { t.Fatal("couldn't save item", err) @@ -255,6 +257,8 @@ func testWebhookStoreGetOutgoing(t *testing.T, ss store.Store) { o1.CreatorId = model.NewId() o1.TeamId = model.NewId() o1.CallbackURLs = []string{"http://nowhere.com/"} + o1.Username = "test-user-name" + o1.IconURL = "http://nowhere.com/icon" o1 = (<-ss.Webhook().SaveOutgoing(o1)).Data.(*model.OutgoingWebhook) @@ -461,10 +465,13 @@ func testWebhookStoreUpdateOutgoing(t *testing.T, ss store.Store) { o1.CreatorId = model.NewId() o1.TeamId = model.NewId() o1.CallbackURLs = []string{"http://nowhere.com/"} + o1.Username = "test-user-name" + o1.IconURL = "http://nowhere.com/icon" o1 = (<-ss.Webhook().SaveOutgoing(o1)).Data.(*model.OutgoingWebhook) o1.Token = model.NewId() + o1.Username = "another-test-user-name" if r2 := <-ss.Webhook().UpdateOutgoing(o1); r2.Err != nil { t.Fatal(r2.Err) -- cgit v1.2.3-1-g7c22