diff options
author | Jesse Hallam <jesse.hallam@gmail.com> | 2018-01-02 11:41:23 -0500 |
---|---|---|
committer | Christopher Speller <crspeller@gmail.com> | 2018-01-02 08:41:23 -0800 |
commit | e9fe9f50dd5839a7cf0926a2d97e88a19c9b831e (patch) | |
tree | 87073ea7310a356b4f2d78a5b5f569730ef89257 | |
parent | b902e4eea9732573d3a3dc9f6a62ca7029cac409 (diff) | |
download | chat-e9fe9f50dd5839a7cf0926a2d97e88a19c9b831e.tar.gz chat-e9fe9f50dd5839a7cf0926a2d97e88a19c9b831e.tar.bz2 chat-e9fe9f50dd5839a7cf0926a2d97e88a19c9b831e.zip |
[PLT-8173] Add username and profile picture to webhook set up pages (#8002)
-rw-r--r-- | api4/webhook_test.go | 61 | ||||
-rw-r--r-- | app/webhook.go | 33 | ||||
-rw-r--r-- | app/webhook_test.go | 283 | ||||
-rw-r--r-- | i18n/en.json | 12 | ||||
-rw-r--r-- | model/incoming_webhook.go | 28 | ||||
-rw-r--r-- | model/incoming_webhook_test.go | 20 | ||||
-rw-r--r-- | store/sqlstore/upgrade.go | 3 |
7 files changed, 429 insertions, 11 deletions
diff --git a/api4/webhook_test.go b/api4/webhook_test.go index 4a3520bd4..464245259 100644 --- a/api4/webhook_test.go +++ b/api4/webhook_test.go @@ -20,6 +20,8 @@ func TestCreateIncomingWebhook(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableIncomingWebhooks = true }) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableOnlyAdminIntegrations = true }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostUsernameOverride = true }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostIconOverride = true }) hook := &model.IncomingWebhook{ChannelId: th.BasicChannel.Id} @@ -56,6 +58,12 @@ func TestCreateIncomingWebhook(t *testing.T) { _, resp = Client.CreateIncomingWebhook(hook) CheckNoError(t, resp) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostUsernameOverride = false }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostIconOverride = false }) + + _, resp = Client.CreateIncomingWebhook(hook) + CheckNoError(t, resp) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableIncomingWebhooks = false }) _, resp = Client.CreateIncomingWebhook(hook) CheckNotImplementedStatus(t, resp) @@ -410,10 +418,55 @@ func TestUpdateIncomingHook(t *testing.T) { createdHook, resp := th.SystemAdminClient.CreateIncomingWebhook(hook1) CheckNoError(t, resp) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostUsernameOverride = false }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostIconOverride = false }) + + t.Run("UpdateIncomingHook, overrides disabled", func(t *testing.T) { + createdHook.DisplayName = "hook2" + createdHook.Description = "description" + createdHook.ChannelId = th.BasicChannel2.Id + createdHook.PostUsername = "username" + createdHook.PostIconURL = "icon" + + updatedHook, resp := th.SystemAdminClient.UpdateIncomingWebhook(createdHook) + CheckNoError(t, resp) + if updatedHook != nil { + if updatedHook.DisplayName != "hook2" { + t.Fatal("Hook name is not updated") + } + + if updatedHook.Description != "description" { + t.Fatal("Hook description is not updated") + } + + if updatedHook.ChannelId != th.BasicChannel2.Id { + t.Fatal("Hook channel is not updated") + } + + if updatedHook.PostUsername != "" { + t.Fatal("Hook username was incorrectly updated") + } + + if updatedHook.PostIconURL != "" { + t.Fatal("Hook icon was incorrectly updated") + } + } else { + t.Fatal("should not be nil") + } + + //updatedHook, _ = th.App.GetIncomingWebhook(createdHook.Id) + assert.Equal(t, updatedHook.ChannelId, createdHook.ChannelId) + }) + + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostUsernameOverride = true }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostIconOverride = true }) + t.Run("UpdateIncomingHook", func(t *testing.T) { createdHook.DisplayName = "hook2" createdHook.Description = "description" createdHook.ChannelId = th.BasicChannel2.Id + createdHook.PostUsername = "username" + createdHook.PostIconURL = "icon" updatedHook, resp := th.SystemAdminClient.UpdateIncomingWebhook(createdHook) CheckNoError(t, resp) @@ -429,6 +482,14 @@ func TestUpdateIncomingHook(t *testing.T) { if updatedHook.ChannelId != th.BasicChannel2.Id { t.Fatal("Hook channel is not updated") } + + if updatedHook.PostUsername != "username" { + t.Fatal("Hook username is not updated") + } + + if updatedHook.PostIconURL != "icon" { + t.Fatal("Hook icon is not updated") + } } else { t.Fatal("should not be nil") } diff --git a/app/webhook.go b/app/webhook.go index ba513def5..5ed71e992 100644 --- a/app/webhook.go +++ b/app/webhook.go @@ -277,6 +277,17 @@ func (a *App) CreateIncomingWebhookForChannel(creatorId string, channel *model.C hook.UserId = creatorId hook.TeamId = channel.TeamId + if !a.Config().ServiceSettings.EnablePostUsernameOverride { + hook.PostUsername = "" + } + if !a.Config().ServiceSettings.EnablePostIconOverride { + hook.PostIconURL = "" + } + + if hook.PostUsername != "" && !model.IsValidUsername(hook.PostUsername) { + return nil, model.NewAppError("CreateIncomingWebhookForChannel", "api.incoming_webhook.invalid_post_username.app_error", nil, "", http.StatusBadRequest) + } + if result := <-a.Srv.Store.Webhook().SaveIncoming(hook); result.Err != nil { return nil, result.Err } else { @@ -289,6 +300,17 @@ func (a *App) UpdateIncomingWebhook(oldHook, updatedHook *model.IncomingWebhook) return nil, model.NewAppError("UpdateIncomingWebhook", "api.incoming_webhook.disabled.app_error", nil, "", http.StatusNotImplemented) } + if !a.Config().ServiceSettings.EnablePostUsernameOverride { + updatedHook.PostUsername = oldHook.PostUsername + } + if !a.Config().ServiceSettings.EnablePostIconOverride { + updatedHook.PostIconURL = oldHook.PostIconURL + } + + if updatedHook.PostUsername != "" && !model.IsValidUsername(updatedHook.PostUsername) { + return nil, model.NewAppError("UpdateIncomingWebhook", "api.incoming_webhook.invalid_post_username.app_error", nil, "", http.StatusBadRequest) + } + updatedHook.Id = oldHook.Id updatedHook.UserId = oldHook.UserId updatedHook.CreateAt = oldHook.CreateAt @@ -608,8 +630,15 @@ func (a *App) HandleIncomingWebhook(hookId string, req *model.IncomingWebhookReq return model.NewAppError("HandleIncomingWebhook", "web.incoming_webhook.permissions.app_error", nil, "", http.StatusForbidden) } - overrideUsername := req.Username - overrideIconUrl := req.IconURL + overrideUsername := hook.PostUsername + if req.Username != "" { + overrideUsername = req.Username + } + + overrideIconUrl := hook.PostIconURL + if req.IconURL != "" { + overrideIconUrl = req.IconURL + } _, err := a.CreateWebhookPost(hook.UserId, channel, text, overrideUsername, overrideIconUrl, req.Props, webhookType, "") return err diff --git a/app/webhook_test.go b/app/webhook_test.go index bc72d4b6e..048c8361d 100644 --- a/app/webhook_test.go +++ b/app/webhook_test.go @@ -13,6 +13,289 @@ import ( "github.com/mattermost/mattermost-server/model" ) +func TestCreateIncomingWebhookForChannel(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + enableIncomingHooks := th.App.Config().ServiceSettings.EnableIncomingWebhooks + defer th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableIncomingWebhooks = enableIncomingHooks }) + enablePostUsernameOverride := th.App.Config().ServiceSettings.EnablePostUsernameOverride + defer th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostUsernameOverride = enablePostUsernameOverride }) + enablePostIconOverride := th.App.Config().ServiceSettings.EnablePostIconOverride + defer th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostIconOverride = enablePostIconOverride }) + + type TestCase struct { + EnableIncomingHooks bool + EnablePostUsernameOverride bool + EnablePostIconOverride bool + IncomingWebhook model.IncomingWebhook + + ExpectedError bool + ExpectedIncomingWebhook *model.IncomingWebhook + } + + for name, tc := range map[string]TestCase{ + "webhooks not enabled": { + EnableIncomingHooks: false, + EnablePostUsernameOverride: false, + EnablePostIconOverride: false, + IncomingWebhook: model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + }, + + ExpectedError: true, + ExpectedIncomingWebhook: nil, + }, + "valid: username and post icon url ignored, since override not enabled": { + EnableIncomingHooks: true, + EnablePostUsernameOverride: false, + EnablePostIconOverride: false, + IncomingWebhook: model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + PostUsername: ":invalid and ignored:", + PostIconURL: "ignored", + }, + + ExpectedError: false, + ExpectedIncomingWebhook: &model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + }, + }, + "invalid username, override enabled": { + EnableIncomingHooks: true, + EnablePostUsernameOverride: true, + EnablePostIconOverride: false, + IncomingWebhook: model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + PostUsername: ":invalid:", + }, + + ExpectedError: true, + ExpectedIncomingWebhook: nil, + }, + "valid, no username or post icon url provided": { + EnableIncomingHooks: true, + EnablePostUsernameOverride: true, + EnablePostIconOverride: true, + IncomingWebhook: model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + }, + + ExpectedError: false, + ExpectedIncomingWebhook: &model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + }, + }, + "valid, with username and post icon": { + EnableIncomingHooks: true, + EnablePostUsernameOverride: true, + EnablePostIconOverride: true, + IncomingWebhook: model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + PostUsername: "valid", + PostIconURL: "http://example.com/icon", + }, + + ExpectedError: false, + ExpectedIncomingWebhook: &model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + PostUsername: "valid", + PostIconURL: "http://example.com/icon", + }, + }, + } { + t.Run(name, func(t *testing.T) { + assert := assert.New(t) + + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableIncomingWebhooks = tc.EnableIncomingHooks }) + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.ServiceSettings.EnablePostUsernameOverride = tc.EnablePostUsernameOverride + }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostIconOverride = tc.EnablePostIconOverride }) + + createdHook, err := th.App.CreateIncomingWebhookForChannel(th.BasicUser.Id, th.BasicChannel, &tc.IncomingWebhook) + if tc.ExpectedError && err == nil { + t.Fatal("should have failed") + } else if !tc.ExpectedError && err != nil { + t.Fatalf("should not have failed: %v", err.Error()) + } + if createdHook != nil { + defer th.App.DeleteIncomingWebhook(createdHook.Id) + } + if tc.ExpectedIncomingWebhook == nil { + assert.Nil(createdHook, "expected nil webhook") + } else if assert.NotNil(createdHook, "expected non-nil webhook") { + assert.Equal(tc.ExpectedIncomingWebhook.DisplayName, createdHook.DisplayName) + assert.Equal(tc.ExpectedIncomingWebhook.Description, createdHook.Description) + assert.Equal(tc.ExpectedIncomingWebhook.ChannelId, createdHook.ChannelId) + assert.Equal(tc.ExpectedIncomingWebhook.PostUsername, createdHook.PostUsername) + assert.Equal(tc.ExpectedIncomingWebhook.PostIconURL, createdHook.PostIconURL) + } + }) + } +} + +func TestUpdateIncomingWebhook(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + enableIncomingHooks := th.App.Config().ServiceSettings.EnableIncomingWebhooks + defer th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableIncomingWebhooks = enableIncomingHooks }) + enablePostUsernameOverride := th.App.Config().ServiceSettings.EnablePostUsernameOverride + defer th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostUsernameOverride = enablePostUsernameOverride }) + enablePostIconOverride := th.App.Config().ServiceSettings.EnablePostIconOverride + defer th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostIconOverride = enablePostIconOverride }) + + type TestCase struct { + EnableIncomingHooks bool + EnablePostUsernameOverride bool + EnablePostIconOverride bool + IncomingWebhook model.IncomingWebhook + + ExpectedError bool + ExpectedIncomingWebhook *model.IncomingWebhook + } + + for name, tc := range map[string]TestCase{ + "webhooks not enabled": { + EnableIncomingHooks: false, + EnablePostUsernameOverride: false, + EnablePostIconOverride: false, + IncomingWebhook: model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + }, + + ExpectedError: true, + ExpectedIncomingWebhook: nil, + }, + "valid: username and post icon url ignored, since override not enabled": { + EnableIncomingHooks: true, + EnablePostUsernameOverride: false, + EnablePostIconOverride: false, + IncomingWebhook: model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + PostUsername: ":invalid and ignored:", + PostIconURL: "ignored", + }, + + ExpectedError: false, + ExpectedIncomingWebhook: &model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + }, + }, + "invalid username, override enabled": { + EnableIncomingHooks: true, + EnablePostUsernameOverride: true, + EnablePostIconOverride: false, + IncomingWebhook: model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + PostUsername: ":invalid:", + }, + + ExpectedError: true, + ExpectedIncomingWebhook: nil, + }, + "valid, no username or post icon url provided": { + EnableIncomingHooks: true, + EnablePostUsernameOverride: true, + EnablePostIconOverride: true, + IncomingWebhook: model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + }, + + ExpectedError: false, + ExpectedIncomingWebhook: &model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + }, + }, + "valid, with username and post icon": { + EnableIncomingHooks: true, + EnablePostUsernameOverride: true, + EnablePostIconOverride: true, + IncomingWebhook: model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + PostUsername: "valid", + PostIconURL: "http://example.com/icon", + }, + + ExpectedError: false, + ExpectedIncomingWebhook: &model.IncomingWebhook{ + DisplayName: "title", + Description: "description", + ChannelId: th.BasicChannel.Id, + PostUsername: "valid", + PostIconURL: "http://example.com/icon", + }, + }, + } { + t.Run(name, func(t *testing.T) { + assert := assert.New(t) + + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableIncomingWebhooks = true }) + + hook, err := th.App.CreateIncomingWebhookForChannel(th.BasicUser.Id, th.BasicChannel, &model.IncomingWebhook{ + ChannelId: th.BasicChannel.Id, + }) + if err != nil { + t.Fatal(err.Error()) + } + defer th.App.DeleteIncomingWebhook(hook.Id) + + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnableIncomingWebhooks = tc.EnableIncomingHooks }) + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.ServiceSettings.EnablePostUsernameOverride = tc.EnablePostUsernameOverride + }) + th.App.UpdateConfig(func(cfg *model.Config) { cfg.ServiceSettings.EnablePostIconOverride = tc.EnablePostIconOverride }) + + updatedHook, err := th.App.UpdateIncomingWebhook(hook, &tc.IncomingWebhook) + if tc.ExpectedError && err == nil { + t.Fatal("should have failed") + } else if !tc.ExpectedError && err != nil { + t.Fatalf("should not have failed: %v", err.Error()) + } + if tc.ExpectedIncomingWebhook == nil { + assert.Nil(updatedHook, "expected nil webhook") + } else if assert.NotNil(updatedHook, "expected non-nil webhook") { + assert.Equal(tc.ExpectedIncomingWebhook.DisplayName, updatedHook.DisplayName) + assert.Equal(tc.ExpectedIncomingWebhook.Description, updatedHook.Description) + assert.Equal(tc.ExpectedIncomingWebhook.ChannelId, updatedHook.ChannelId) + assert.Equal(tc.ExpectedIncomingWebhook.PostUsername, updatedHook.PostUsername) + assert.Equal(tc.ExpectedIncomingWebhook.PostIconURL, updatedHook.PostIconURL) + } + }) + } +} + func TestCreateWebhookPost(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() diff --git a/i18n/en.json b/i18n/en.json index d687449b8..f82d86af9 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1425,6 +1425,10 @@ "translation": "Incoming webhooks have been disabled by the system admin." }, { + "id": "api.incoming_webhook.invalid_post_username.app_error", + "translation": "Invalid username." + }, + { "id": "api.ldap.init.debug", "translation": "Initializing LDAP API routes" }, @@ -4979,6 +4983,14 @@ "translation": "Invalid Id" }, { + "id": "model.incoming_hook.username.app_error", + "translation": "Invalid username" + }, + { + "id": "model.incoming_hook.post_icon_url.app_error", + "translation": "Invalid post icon" + }, + { "id": "model.incoming_hook.team_id.app_error", "translation": "Invalid team ID" }, diff --git a/model/incoming_webhook.go b/model/incoming_webhook.go index e9468e87b..2eb007340 100644 --- a/model/incoming_webhook.go +++ b/model/incoming_webhook.go @@ -16,15 +16,17 @@ const ( ) type IncomingWebhook struct { - Id string `json:"id"` - CreateAt int64 `json:"create_at"` - UpdateAt int64 `json:"update_at"` - DeleteAt int64 `json:"delete_at"` - UserId string `json:"user_id"` - ChannelId string `json:"channel_id"` - TeamId string `json:"team_id"` - DisplayName string `json:"display_name"` - Description string `json:"description"` + Id string `json:"id"` + CreateAt int64 `json:"create_at"` + UpdateAt int64 `json:"update_at"` + DeleteAt int64 `json:"delete_at"` + UserId string `json:"user_id"` + ChannelId string `json:"channel_id"` + TeamId string `json:"team_id"` + DisplayName string `json:"display_name"` + Description string `json:"description"` + PostUsername string `json:"post_username"` + PostIconURL string `json:"post_icon_url"` } type IncomingWebhookRequest struct { @@ -112,6 +114,14 @@ func (o *IncomingWebhook) IsValid() *AppError { return NewAppError("IncomingWebhook.IsValid", "model.incoming_hook.description.app_error", nil, "", http.StatusBadRequest) } + if len(o.PostUsername) > 64 { + return NewAppError("IncomingWebhook.IsValid", "model.incoming_hook.username.app_error", nil, "", http.StatusBadRequest) + } + + if len(o.PostIconURL) > 1024 { + return NewAppError("IncomingWebhook.IsValid", "model.incoming_hook.post_icon_url.app_error", nil, "", http.StatusBadRequest) + } + return nil } diff --git a/model/incoming_webhook_test.go b/model/incoming_webhook_test.go index 5b78f877f..13b416eb0 100644 --- a/model/incoming_webhook_test.go +++ b/model/incoming_webhook_test.go @@ -89,6 +89,26 @@ func TestIncomingWebhookIsValid(t *testing.T) { if err := o.IsValid(); err != nil { t.Fatal(err) } + + o.PostUsername = strings.Repeat("1", 65) + if err := o.IsValid(); err == nil { + t.Fatal("should be invalid") + } + + o.PostUsername = strings.Repeat("1", 64) + if err := o.IsValid(); err != nil { + t.Fatal(err) + } + + o.PostIconURL = strings.Repeat("1", 1025) + if err := o.IsValid(); err == nil { + t.Fatal("should be invalid") + } + + o.PostIconURL = strings.Repeat("1", 1024) + if err := o.IsValid(); err != nil { + t.Fatal(err) + } } func TestIncomingWebhookPreSave(t *testing.T) { diff --git a/store/sqlstore/upgrade.go b/store/sqlstore/upgrade.go index 4d6985033..8d318ca8a 100644 --- a/store/sqlstore/upgrade.go +++ b/store/sqlstore/upgrade.go @@ -328,6 +328,9 @@ func UpgradeDatabaseToVersion46(sqlStore SqlStore) { //TODO: Uncomment folowing when version 4.6 is released //if shouldPerformUpgrade(sqlStore, VERSION_4_5_0, VERSION_4_6_0) { + sqlStore.CreateColumnIfNotExists("IncomingWebhooks", "PostUsername", "varchar(64)", "varchar(64)", "") + sqlStore.CreateColumnIfNotExists("IncomingWebhooks", "PostIconURL", "varchar(1024)", "varchar(1024)", "") + //saveSchemaVersion(sqlStore, VERSION_4_6_0) //} } |