[PLT-8173] Add username and profile picture to webhook set up pages (#8002)

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,15 @@ func TestUpdateIncomingHook(t *testing.T) {
 	createdHook, resp := th.SystemAdminClient.CreateIncomingWebhook(hook1)
 	CheckNoError(t, resp)
 
-	t.Run("UpdateIncomingHook", func(t *testing.T) {
+	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)
@@ -429,6 +442,54 @@ func TestUpdateIncomingHook(t *testing.T) {
 			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)
+		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 != "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")
 		}

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
+	overrideUsername := hook.PostUsername
-	overrideIconUrl := req.IconURL
+	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

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()

i18n/en.json

@@ -1424,6 +1424,10 @@
     "id": "api.incoming_webhook.disabled.app_error",
     "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"
@@ -4978,6 +4982,14 @@
     "id": "model.incoming_hook.id.app_error",
     "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"

model/incoming_webhook.go

@@ -16,15 +16,17 @@ const (
 )
 
 type IncomingWebhook struct {
-	Id          string `json:"id"`
+	Id           string `json:"id"`
-	CreateAt    int64  `json:"create_at"`
+	CreateAt     int64  `json:"create_at"`
-	UpdateAt    int64  `json:"update_at"`
+	UpdateAt     int64  `json:"update_at"`
-	DeleteAt    int64  `json:"delete_at"`
+	DeleteAt     int64  `json:"delete_at"`
-	UserId      string `json:"user_id"`
+	UserId       string `json:"user_id"`
-	ChannelId   string `json:"channel_id"`
+	ChannelId    string `json:"channel_id"`
-	TeamId      string `json:"team_id"`
+	TeamId       string `json:"team_id"`
-	DisplayName string `json:"display_name"`
+	DisplayName  string `json:"display_name"`
-	Description string `json:"description"`
+	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
 }
 

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) {

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)
 	//}
 }