Adding edit of incoming webhook (#5272)

Adding edit of outgoing webhook
Fixing spelling of error
Fixing style
Changing from PUT to POST for updates
Fixing test failures due to merge

api/webhook.go

@@ -21,10 +21,12 @@ func InitWebhook() {
 	l4g.Debug(utils.T("api.webhook.init.debug"))
 
 	BaseRoutes.Hooks.Handle("/incoming/create", ApiUserRequired(createIncomingHook)).Methods("POST")
+	BaseRoutes.Hooks.Handle("/incoming/update", ApiUserRequired(updateIncomingHook)).Methods("POST")
 	BaseRoutes.Hooks.Handle("/incoming/delete", ApiUserRequired(deleteIncomingHook)).Methods("POST")
 	BaseRoutes.Hooks.Handle("/incoming/list", ApiUserRequired(getIncomingHooks)).Methods("GET")
 
 	BaseRoutes.Hooks.Handle("/outgoing/create", ApiUserRequired(createOutgoingHook)).Methods("POST")
+	BaseRoutes.Hooks.Handle("/outgoing/update", ApiUserRequired(updateOutgoingHook)).Methods("POST")
 	BaseRoutes.Hooks.Handle("/outgoing/regen_token", ApiUserRequired(regenOutgoingHookToken)).Methods("POST")
 	BaseRoutes.Hooks.Handle("/outgoing/delete", ApiUserRequired(deleteOutgoingHook)).Methods("POST")
 	BaseRoutes.Hooks.Handle("/outgoing/list", ApiUserRequired(getOutgoingHooks)).Methods("GET")
@@ -71,16 +73,86 @@ func createIncomingHook(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func deleteIncomingHook(c *Context, w http.ResponseWriter, r *http.Request) {
-	if !utils.Cfg.ServiceSettings.EnableIncomingWebhooks {
-		c.Err = model.NewLocAppError("deleteIncomingHook", "api.webhook.delete_incoming.disabled.app_errror", nil, "")
-		c.Err.StatusCode = http.StatusNotImplemented
+func updateIncomingHook(c *Context, w http.ResponseWriter, r *http.Request) {
+	if err := checkIncomingWebHooks("updateIncomingHook", "api.webhook.update_incoming.disabled.app_error"); err != nil {
+		c.Err = err
 		return
 	}
 
-	if !app.SessionHasPermissionToTeam(c.Session, c.TeamId, model.PERMISSION_MANAGE_WEBHOOKS) {
-		c.Err = model.NewLocAppError("deleteIncomingHook", "api.command.admin_only.app_error", nil, "")
-		c.Err.StatusCode = http.StatusForbidden
+	if err := checkManageWebhooksPermission(c, "updateIncomingHook", "api.command.admin_only.app_error"); err != nil {
+		c.Err = err
+		return
+	}
+
+	c.LogAudit("attempt")
+
+	hook := model.IncomingWebhookFromJson(r.Body)
+
+	if hook == nil {
+		c.SetInvalidParam("updateIncomingHook", "webhook")
+		return
+	}
+
+	var oldHook *model.IncomingWebhook
+	var result store.StoreResult
+
+	if result = <-app.Srv.Store.Webhook().GetIncoming(hook.Id, true); result.Err != nil {
+		c.LogAudit("no existing incoming hook found")
+		c.Err = result.Err
+		return
+	}
+
+	oldHook = result.Data.(*model.IncomingWebhook)
+	cchan := app.Srv.Store.Channel().Get(hook.ChannelId, true)
+
+	var channel *model.Channel
+	if result = <-cchan; result.Err != nil {
+		c.Err = result.Err
+		return
+	}
+
+	channel = result.Data.(*model.Channel)
+	if channel.Type != model.CHANNEL_OPEN && !app.SessionHasPermissionToChannel(c.Session, channel.Id, model.PERMISSION_READ_CHANNEL) {
+		c.LogAudit("fail - bad channel permissions")
+		c.SetPermissionError(model.PERMISSION_READ_CHANNEL)
+		return
+	}
+
+	if c.Session.UserId != oldHook.UserId && !app.SessionHasPermissionToTeam(c.Session, c.TeamId, model.PERMISSION_MANAGE_OTHERS_WEBHOOKS) {
+		c.LogAudit("fail - inappropriate permissions")
+		c.Err = model.NewLocAppError("updateIncomingHook", "api.webhook.update_incoming.permissions.app_error", nil, "user_id="+c.Session.UserId)
+		return
+	}
+
+	if c.TeamId != oldHook.TeamId {
+		c.Err = model.NewLocAppError("UpdateIncomingHook", "api.webhook.team_mismatch.app_error", nil, "user_id="+c.Session.UserId)
+		return
+	}
+
+	hook.UserId = oldHook.UserId
+	hook.CreateAt = oldHook.CreateAt
+	hook.UpdateAt = model.GetMillis()
+	hook.TeamId = oldHook.TeamId
+	hook.DeleteAt = oldHook.DeleteAt
+
+	if result = <-app.Srv.Store.Webhook().UpdateIncoming(hook); result.Err != nil {
+		c.Err = result.Err
+		return
+	}
+
+	c.LogAudit("success")
+	rhook := result.Data.(*model.IncomingWebhook)
+	w.Write([]byte(rhook.ToJson()))
+}
+
+func deleteIncomingHook(c *Context, w http.ResponseWriter, r *http.Request) {
+	if err := checkIncomingWebHooks("deleteIncomingHook", "api.webhook.delete_incoming.disabled.app_error"); err != nil {
+		c.Err = err
+		return
+	}
+
+	if err := checkManageWebhooksPermission(c, "deleteIncomingHook", "api.command.admin_only.app_error"); err != nil {
+		c.Err = err
 		return
 	}
 
@@ -100,7 +172,7 @@ func deleteIncomingHook(c *Context, w http.ResponseWriter, r *http.Request) {
 	} else {
 		if c.Session.UserId != result.Data.(*model.IncomingWebhook).UserId && !app.SessionHasPermissionToTeam(c.Session, c.TeamId, model.PERMISSION_MANAGE_OTHERS_WEBHOOKS) {
 			c.LogAudit("fail - inappropriate permissions")
-			c.Err = model.NewLocAppError("deleteIncomingHook", "api.webhook.delete_incoming.permissions.app_errror", nil, "user_id="+c.Session.UserId)
+			c.Err = model.NewLocAppError("deleteIncomingHook", "api.webhook.delete_incoming.permissions.app_error", nil, "user_id="+c.Session.UserId)
 			return
 		}
 	}
@@ -130,16 +202,72 @@ func getIncomingHooks(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func createOutgoingHook(c *Context, w http.ResponseWriter, r *http.Request) {
+func checkOutgoingWebHooks(where string, id string) *model.AppError {
 	if !utils.Cfg.ServiceSettings.EnableOutgoingWebhooks {
-		c.Err = model.NewLocAppError("createOutgoingHook", "api.webhook.create_outgoing.disabled.app_error", nil, "")
-		c.Err.StatusCode = http.StatusNotImplemented
+		err := model.NewLocAppError(where, id, nil, "")
+		err.StatusCode = http.StatusNotImplemented
+		return err
+	}
+
+	return nil
+}
+
+func checkIncomingWebHooks(where string, id string) *model.AppError {
+	if !utils.Cfg.ServiceSettings.EnableIncomingWebhooks {
+		err := model.NewLocAppError(where, id, nil, "")
+		err.StatusCode = http.StatusNotImplemented
+		return err
+	}
+
+	return nil
+}
+
+func checkManageWebhooksPermission(c *Context, where string, id string) *model.AppError {
+	if !app.SessionHasPermissionToTeam(c.Session, c.TeamId, model.PERMISSION_MANAGE_WEBHOOKS) {
+		err := model.NewLocAppError(where, id, nil, "")
+		err.StatusCode = http.StatusForbidden
+		return err
+	}
+
+	return nil
+}
+
+func checkValidOutgoingHook(hook *model.OutgoingWebhook, c *Context, where string, id string) *model.AppError {
+	if len(hook.ChannelId) != 0 {
+		cchan := app.Srv.Store.Channel().Get(hook.ChannelId, true)
+
+		var channel *model.Channel
+		var result store.StoreResult
+		if result = <-cchan; result.Err != nil {
+			return result.Err
+		}
+
+		channel = result.Data.(*model.Channel)
+
+		if channel.Type != model.CHANNEL_OPEN {
+			c.LogAudit("fail - not open channel")
+			return model.NewLocAppError(where, "api.webhook."+id+".not_open.app_error", nil, "")
+		}
+
+		if channel.TeamId != c.TeamId {
+			c.LogAudit("fail - cannot update command to a different team")
+			return model.NewLocAppError(where, "api.webhook."+id+".permissions.app_error", nil, "")
+		}
+	} else if len(hook.TriggerWords) == 0 {
+		return model.NewLocAppError(where, "api.webhook."+id+".triggers.app_error", nil, "")
+	}
+
+	return nil
+}
+
+func createOutgoingHook(c *Context, w http.ResponseWriter, r *http.Request) {
+	if err := checkOutgoingWebHooks("createOutgoingHook", "api.webhook.create_outgoing.disabled.app_error"); err != nil {
+		c.Err = err
 		return
 	}
 
-	if !app.SessionHasPermissionToTeam(c.Session, c.TeamId, model.PERMISSION_MANAGE_WEBHOOKS) {
-		c.Err = model.NewLocAppError("createOutgoingHook", "api.command.admin_only.app_error", nil, "")
-		c.Err.StatusCode = http.StatusForbidden
+	if err := checkManageWebhooksPermission(c, "createOutgoingHook", "api.command.admin_only.app_error"); err != nil {
+		c.Err = err
 		return
 	}
 
@@ -155,30 +283,8 @@ func createOutgoingHook(c *Context, w http.ResponseWriter, r *http.Request) {
 	hook.CreatorId = c.Session.UserId
 	hook.TeamId = c.TeamId
 
-	if len(hook.ChannelId) != 0 {
-		cchan := app.Srv.Store.Channel().Get(hook.ChannelId, true)
-
-		var channel *model.Channel
-		if result := <-cchan; result.Err != nil {
-			c.Err = result.Err
-			return
-		} else {
-			channel = result.Data.(*model.Channel)
-		}
-
-		if channel.Type != model.CHANNEL_OPEN {
-			c.LogAudit("fail - not open channel")
-			c.Err = model.NewLocAppError("createOutgoingHook", "api.webhook.create_outgoing.not_open.app_error", nil, "")
-			return
-		}
-
-		if channel.Type != model.CHANNEL_OPEN || channel.TeamId != c.TeamId {
-			c.LogAudit("fail - bad channel permissions")
-			c.Err = model.NewLocAppError("createOutgoingHook", "api.webhook.create_outgoing.permissions.app_error", nil, "")
-			return
-		}
-	} else if len(hook.TriggerWords) == 0 {
-		c.Err = model.NewLocAppError("createOutgoingHook", "api.webhook.create_outgoing.triggers.app_error", nil, "")
+	if err := checkValidOutgoingHook(hook, c, "createOutgoingHook", "create_outgoing"); err != nil {
+		c.Err = err
 		return
 	}
 
@@ -210,15 +316,13 @@ func createOutgoingHook(c *Context, w http.ResponseWriter, r *http.Request) {
 }
 
 func getOutgoingHooks(c *Context, w http.ResponseWriter, r *http.Request) {
-	if !utils.Cfg.ServiceSettings.EnableOutgoingWebhooks {
-		c.Err = model.NewLocAppError("getOutgoingHooks", "api.webhook.get_outgoing.disabled.app_error", nil, "")
-		c.Err.StatusCode = http.StatusNotImplemented
+	if err := checkOutgoingWebHooks("getOutgoingHooks", "api.webhook.get_outgoing.disabled.app_error"); err != nil {
+		c.Err = err
 		return
 	}
 
-	if !app.SessionHasPermissionToTeam(c.Session, c.TeamId, model.PERMISSION_MANAGE_WEBHOOKS) {
-		c.Err = model.NewLocAppError("getOutgoingHooks", "api.command.admin_only.app_error", nil, "")
-		c.Err.StatusCode = http.StatusForbidden
+	if err := checkManageWebhooksPermission(c, "getOutgoingHooks", "api.command.admin_only.app_error"); err != nil {
+		c.Err = err
 		return
 	}
 
@@ -231,16 +335,85 @@ func getOutgoingHooks(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func deleteOutgoingHook(c *Context, w http.ResponseWriter, r *http.Request) {
-	if !utils.Cfg.ServiceSettings.EnableOutgoingWebhooks {
-		c.Err = model.NewLocAppError("deleteOutgoingHook", "api.webhook.delete_outgoing.disabled.app_error", nil, "")
-		c.Err.StatusCode = http.StatusNotImplemented
+func updateOutgoingHook(c *Context, w http.ResponseWriter, r *http.Request) {
+	if err := checkOutgoingWebHooks("updateOutgoingHook", "api.webhook.update_outgoing.disabled.app_error"); err != nil {
+		c.Err = err
 		return
 	}
 
-	if !app.SessionHasPermissionToTeam(c.Session, c.TeamId, model.PERMISSION_MANAGE_WEBHOOKS) {
-		c.Err = model.NewLocAppError("deleteOutgoingHook", "api.command.admin_only.app_error", nil, "")
-		c.Err.StatusCode = http.StatusForbidden
+	if err := checkManageWebhooksPermission(c, "updateOutgoingHook", "api.command.admin_only.app_error"); err != nil {
+		c.Err = err
+		return
+	}
+
+	c.LogAudit("attempt")
+
+	hook := model.OutgoingWebhookFromJson(r.Body)
+
+	if hook == nil {
+		c.SetInvalidParam("updateOutgoingHook", "webhook")
+		return
+	}
+
+	if err := checkValidOutgoingHook(hook, c, "updateOutgoingHook", "update_outgoing"); err != nil {
+		c.Err = err
+		return
+	}
+
+	var result store.StoreResult
+	if result = <-app.Srv.Store.Webhook().GetOutgoingByTeam(c.TeamId); result.Err != nil {
+		c.Err = result.Err
+		return
+	}
+
+	allHooks := result.Data.([]*model.OutgoingWebhook)
+
+	for _, existingOutHook := range allHooks {
+		urlIntersect := utils.StringArrayIntersection(existingOutHook.CallbackURLs, hook.CallbackURLs)
+		triggerIntersect := utils.StringArrayIntersection(existingOutHook.TriggerWords, hook.TriggerWords)
+
+		if existingOutHook.ChannelId == hook.ChannelId && len(urlIntersect) != 0 && len(triggerIntersect) != 0 && existingOutHook.Id != hook.Id {
+			c.Err = model.NewLocAppError("updateOutgoingHook", "api.webhook.update_outgoing.intersect.app_error", nil, "")
+			return
+		}
+	}
+
+	if result = <-app.Srv.Store.Webhook().GetOutgoing(hook.Id); result.Err != nil {
+		c.LogAudit("fail - no existing outgoing webhook found")
+		c.Err = result.Err
+		return
+	}
+
+	oldHook := result.Data.(*model.OutgoingWebhook)
+	if c.TeamId != oldHook.TeamId {
+		c.Err = model.NewLocAppError("UpdateOutgoingHook", "api.webhook.team_mismatch.app_error", nil, "user_id="+c.Session.UserId)
+		return
+	}
+
+	hook.CreatorId = oldHook.CreatorId
+	hook.CreateAt = oldHook.CreateAt
+	hook.DeleteAt = oldHook.DeleteAt
+	hook.TeamId = oldHook.TeamId
+	hook.UpdateAt = model.GetMillis()
+
+	if result = <-app.Srv.Store.Webhook().UpdateOutgoing(hook); result.Err != nil {
+		c.Err = result.Err
+		return
+	}
+
+	c.LogAudit("success")
+	rhook := result.Data.(*model.OutgoingWebhook)
+	w.Write([]byte(rhook.ToJson()))
+}
+
+func deleteOutgoingHook(c *Context, w http.ResponseWriter, r *http.Request) {
+	if err := checkOutgoingWebHooks("deleteOutgoingHook", "api.webhook.delete_outgoing.disabled.app_error"); err != nil {
+		c.Err = err
+		return
+	}
+
+	if err := checkManageWebhooksPermission(c, "deleteOutgoingHook", "api.command.admin_only.app_error"); err != nil {
+		c.Err = err
 		return
 	}
 
@@ -275,15 +448,13 @@ func deleteOutgoingHook(c *Context, w http.ResponseWriter, r *http.Request) {
 }
 
 func regenOutgoingHookToken(c *Context, w http.ResponseWriter, r *http.Request) {
-	if !utils.Cfg.ServiceSettings.EnableOutgoingWebhooks {
-		c.Err = model.NewLocAppError("regenOutgoingHookToken", "api.webhook.regen_outgoing_token.disabled.app_error", nil, "")
-		c.Err.StatusCode = http.StatusNotImplemented
+	if err := checkOutgoingWebHooks("regenOutgoingHookToken", "api.webhook.regen_outgoing_token.disabled.app_error"); err != nil {
+		c.Err = err
 		return
 	}
 
-	if !app.SessionHasPermissionToTeam(c.Session, c.TeamId, model.PERMISSION_MANAGE_WEBHOOKS) {
-		c.Err = model.NewLocAppError("regenOutgoingHookToken", "api.command.admin_only.app_error", nil, "")
-		c.Err.StatusCode = http.StatusForbidden
+	if err := checkManageWebhooksPermission(c, "regenOutgoingHookToken", "api.command.admin_only.app_error"); err != nil {
+		c.Err = err
 		return
 	}
 
@@ -322,9 +493,8 @@ func regenOutgoingHookToken(c *Context, w http.ResponseWriter, r *http.Request)
 }
 
 func incomingWebhook(c *Context, w http.ResponseWriter, r *http.Request) {
-	if !utils.Cfg.ServiceSettings.EnableIncomingWebhooks {
-		c.Err = model.NewLocAppError("incomingWebhook", "web.incoming_webhook.disabled.app_error", nil, "")
-		c.Err.StatusCode = http.StatusNotImplemented
+	if err := checkIncomingWebHooks("incomingWebhook", "web.incoming_webhook.disabled.app_error"); err != nil {
+		c.Err = err
 		return
 	}
 

api/webhook_test.go

@@ -5,10 +5,11 @@ package api
 
 import (
 	"fmt"
-	"github.com/mattermost/platform/model"
-	"github.com/mattermost/platform/utils"
 	"net/http"
 	"testing"
+
+	"github.com/mattermost/platform/model"
+	"github.com/mattermost/platform/utils"
 )
 
 func TestCreateIncomingHook(t *testing.T) {
@@ -117,6 +118,212 @@ func TestCreateIncomingHook(t *testing.T) {
 	}
 }
 
+func TestUpdateIncomingHook(t *testing.T) {
+	th := Setup().InitSystemAdmin()
+	Client := th.SystemAdminClient
+	team := th.SystemAdminTeam
+
+	channel1 := th.CreateChannel(Client, team)
+	channel2 := th.CreatePrivateChannel(Client, team)
+	channel3 := th.CreateChannel(Client, team)
+
+	user2 := th.CreateUser(Client)
+	LinkUserToTeam(user2, team)
+
+	team2 := th.CreateTeam(Client)
+	user3 := th.CreateUser(Client)
+	LinkUserToTeam(user3, team2)
+	UpdateUserToTeamAdmin(user3, team2)
+
+	enableIncomingHooks := utils.Cfg.ServiceSettings.EnableIncomingWebhooks
+	enableAdminOnlyHooks := utils.Cfg.ServiceSettings.EnableOnlyAdminIntegrations
+	defer func() {
+		utils.Cfg.ServiceSettings.EnableIncomingWebhooks = enableIncomingHooks
+		utils.Cfg.ServiceSettings.EnableOnlyAdminIntegrations = enableAdminOnlyHooks
+		utils.SetDefaultRolesBasedOnConfig()
+	}()
+
+	utils.Cfg.ServiceSettings.EnableIncomingWebhooks = true
+	*utils.Cfg.ServiceSettings.EnableOnlyAdminIntegrations = true
+	utils.SetDefaultRolesBasedOnConfig()
+
+	hook := createIncomingWebhook(channel1.Id, Client, t)
+
+	t.Run("UpdateIncomingHook", func(t *testing.T) {
+		hook.DisplayName = "hook2"
+		hook.Description = "description"
+		hook.ChannelId = channel3.Id
+
+		if result, err := Client.UpdateIncomingWebhook(hook); err != nil {
+			t.Fatal("Update hook should not fail")
+		} else {
+			updatedHook := result.Data.(*model.IncomingWebhook)
+
+			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 != channel3.Id {
+				t.Fatal("Hook channel is not updated")
+			}
+		}
+	})
+
+	t.Run("RetainCreateAt", func(t *testing.T) {
+		hook2 := &model.IncomingWebhook{ChannelId: channel1.Id, CreateAt: 100}
+
+		if result, err := Client.CreateIncomingWebhook(hook2); err != nil {
+			t.Fatal("hook creation failed")
+		} else {
+			createdHook := result.Data.(*model.IncomingWebhook)
+			createdHook.DisplayName = "Name2"
+
+			if result, err := Client.UpdateIncomingWebhook(createdHook); err != nil {
+				t.Fatal("Update hook should not fail")
+			} else {
+				updatedHook := result.Data.(*model.IncomingWebhook)
+
+				if updatedHook.CreateAt != createdHook.CreateAt {
+					t.Fatal("failed - hook create at should not be changed")
+				}
+			}
+		}
+	})
+
+	t.Run("ModifyUpdateAt", func(t *testing.T) {
+		hook.DisplayName = "Name3"
+
+		if result, err := Client.UpdateIncomingWebhook(hook); err != nil {
+			t.Fatal("Update hook should not fail")
+		} else {
+			updatedHook := result.Data.(*model.IncomingWebhook)
+
+			if updatedHook.UpdateAt == hook.UpdateAt {
+				t.Fatal("failed - hook updateAt is not updated")
+			}
+		}
+	})
+
+	t.Run("UpdateNonExistentHook", func(t *testing.T) {
+		nonExistentHook := &model.IncomingWebhook{ChannelId: channel1.Id}
+
+		if _, err := Client.UpdateIncomingWebhook(nonExistentHook); err == nil {
+			t.Fatal("should have failed - update a non-existent hook")
+		}
+	})
+
+	Client.Logout()
+	Client.Must(Client.LoginById(user2.Id, user2.Password))
+	Client.SetTeamId(team.Id)
+	t.Run("UserIsNotAdminOfTeam", func(t *testing.T) {
+		if _, err := Client.UpdateIncomingWebhook(hook); err == nil {
+			t.Fatal("should have failed - user is not admin of team")
+		}
+	})
+
+	utils.Cfg.ServiceSettings.EnableIncomingWebhooks = true
+
+	t.Run("OnlyAdminIntegrationsDisabled", func(t *testing.T) {
+		*utils.Cfg.ServiceSettings.EnableOnlyAdminIntegrations = false
+		utils.SetDefaultRolesBasedOnConfig()
+
+		t.Run("UpdateHookOfSameUser", func(t *testing.T) {
+			sameUserHook := &model.IncomingWebhook{ChannelId: channel1.Id, UserId: user2.Id}
+			if result, err := Client.CreateIncomingWebhook(sameUserHook); err != nil {
+				t.Fatal("Hook creation failed")
+			} else {
+				sameUserHook = result.Data.(*model.IncomingWebhook)
+			}
+
+			if _, err := Client.UpdateIncomingWebhook(sameUserHook); err != nil {
+				t.Fatal("should not fail - only admin integrations are disabled & hook of same user")
+			}
+		})
+
+		t.Run("UpdateHookOfDifferentUser", func(t *testing.T) {
+			if _, err := Client.UpdateIncomingWebhook(hook); err == nil {
+				t.Fatal("should have failed - user does not have permissions to update other user's hooks")
+			}
+		})
+	})
+
+	*utils.Cfg.ServiceSettings.EnableOnlyAdminIntegrations = true
+	utils.SetDefaultRolesBasedOnConfig()
+
+	Client.Logout()
+	UpdateUserToTeamAdmin(user2, team)
+	Client.Must(Client.LoginById(user2.Id, user2.Password))
+	Client.SetTeamId(team.Id)
+	t.Run("UpdateByDifferentUser", func(t *testing.T) {
+		if result, err := Client.UpdateIncomingWebhook(hook); err != nil {
+			t.Fatal("Update hook should not fail")
+		} else {
+			updatedHook := result.Data.(*model.IncomingWebhook)
+
+			if updatedHook.UserId == user2.Id {
+				t.Fatal("Hook's creator userId is not retained")
+			}
+		}
+	})
+
+	t.Run("IncomingHooksDisabled", func(t *testing.T) {
+		utils.Cfg.ServiceSettings.EnableIncomingWebhooks = false
+		if _, err := Client.UpdateIncomingWebhook(hook); err == nil {
+			t.Fatal("should have failed - incoming hooks are disabled")
+		}
+	})
+
+	t.Run("PrivateChannel", func(t *testing.T) {
+		hook.ChannelId = channel2.Id
+
+		if _, err := Client.UpdateIncomingWebhook(hook); err == nil {
+			t.Fatal("should have failed - updating to a private channel where the user is not a member")
+		}
+	})
+
+	t.Run("UpdateToNonExistentChannel", func(t *testing.T) {
+		hook.ChannelId = "junk"
+		if _, err := Client.UpdateIncomingWebhook(hook); err == nil {
+			t.Fatal("should have failed - bad channel id")
+		}
+	})
+
+	Client.Logout()
+	Client.Must(Client.LoginById(user3.Id, user3.Password))
+	Client.SetTeamId(team2.Id)
+	t.Run("UpdateToADifferentTeam", func(t *testing.T) {
+		if _, err := Client.UpdateIncomingWebhook(hook); err == nil {
+			t.Fatal("should have failed - update to a different team is not allowed")
+		}
+	})
+}
+
+func createIncomingWebhook(channelID string, Client *model.Client, t *testing.T) *model.IncomingWebhook {
+	hook := &model.IncomingWebhook{ChannelId: channelID}
+	if result, err := Client.CreateIncomingWebhook(hook); err != nil {
+		t.Fatal("Hook creation failed")
+	} else {
+		hook = result.Data.(*model.IncomingWebhook)
+	}
+
+	return hook
+}
+
+func createOutgoingWebhook(channelID string, callbackURLs []string, triggerWords []string, Client *model.Client, t *testing.T) *model.OutgoingWebhook {
+	hook := &model.OutgoingWebhook{ChannelId: channelID, CallbackURLs: callbackURLs, TriggerWords: triggerWords}
+	if result, err := Client.CreateOutgoingWebhook(hook); err != nil {
+		t.Fatal("Hook creation failed")
+	} else {
+		hook = result.Data.(*model.OutgoingWebhook)
+	}
+
+	return hook
+}
+
 func TestListIncomingHooks(t *testing.T) {
 	th := Setup().InitSystemAdmin()
 	Client := th.SystemAdminClient
@@ -416,6 +623,190 @@ func TestListOutgoingHooks(t *testing.T) {
 	}
 }
 
+func TestUpdateOutgoingHook(t *testing.T) {
+	th := Setup().InitSystemAdmin()
+	Client := th.SystemAdminClient
+	user := th.SystemAdminUser
+	team := th.SystemAdminTeam
+	team2 := th.CreateTeam(Client)
+	channel1 := th.CreateChannel(Client, team)
+	channel2 := th.CreatePrivateChannel(Client, team)
+	channel3 := th.CreateChannel(Client, team)
+	user2 := th.CreateUser(Client)
+	LinkUserToTeam(user2, team)
+	user3 := th.CreateUser(Client)
+	LinkUserToTeam(user3, team2)
+
+	enableOutgoingHooks := utils.Cfg.ServiceSettings.EnableOutgoingWebhooks
+	enableAdminOnlyHooks := utils.Cfg.ServiceSettings.EnableOnlyAdminIntegrations
+	defer func() {
+		utils.Cfg.ServiceSettings.EnableOutgoingWebhooks = enableOutgoingHooks
+		utils.Cfg.ServiceSettings.EnableOnlyAdminIntegrations = enableAdminOnlyHooks
+		utils.SetDefaultRolesBasedOnConfig()
+	}()
+
+	utils.Cfg.ServiceSettings.EnableOutgoingWebhooks = true
+	*utils.Cfg.ServiceSettings.EnableOnlyAdminIntegrations = true
+	utils.SetDefaultRolesBasedOnConfig()
+
+	hook := createOutgoingWebhook(channel1.Id, []string{"http://nowhere.com"}, []string{"cats"}, Client, t)
+	createOutgoingWebhook(channel1.Id, []string{"http://nowhere.com"}, []string{"dogs"}, Client, t)
+
+	hook.DisplayName = "Cats"
+	hook.Description = "Get me some cats"
+	t.Run("OutgoingHooksDisabled", func(t *testing.T) {
+		utils.Cfg.ServiceSettings.EnableOutgoingWebhooks = false
+		if _, err := Client.UpdateOutgoingWebhook(hook); err == nil {
+			t.Fatal("should have failed - outgoing webhooks disabled")
+		}
+	})
+
+	utils.Cfg.ServiceSettings.EnableOutgoingWebhooks = true
+	t.Run("UpdateOutgoingWebhook", func(t *testing.T) {
+		if result, err := Client.UpdateOutgoingWebhook(hook); err != nil {
+			t.Fatal("failed to update outgoing web hook")
+		} else {
+			updatedHook := result.Data.(*model.OutgoingWebhook)
+
+			if updatedHook.DisplayName != hook.DisplayName {
+				t.Fatal("Hook display name did not get updated")
+			}
+
+			if updatedHook.Description != hook.Description {
+				t.Fatal("Hook description did not get updated")
+			}
+		}
+	})
+
+	t.Run("RetainCreateAt", func(t *testing.T) {
+		hook2 := &model.OutgoingWebhook{ChannelId: channel1.Id, CallbackURLs: []string{"http://nowhere.com"}, TriggerWords: []string{"rats"}}
+
+		if result, err := Client.CreateOutgoingWebhook(hook2); err != nil {
+			t.Fatal("hook creation failed")
+		} else {
+			createdHook := result.Data.(*model.OutgoingWebhook)
+			createdHook.DisplayName = "Name2"
+
+			if result, err := Client.UpdateOutgoingWebhook(createdHook); err != nil {
+				t.Fatal("Update hook should not fail")
+			} else {
+				updatedHook := result.Data.(*model.OutgoingWebhook)
+
+				if updatedHook.CreateAt != createdHook.CreateAt {
+					t.Fatal("failed - hook create at should not be changed")
+				}
+			}
+		}
+	})
+
+	t.Run("ModifyUpdateAt", func(t *testing.T) {
+		hook.DisplayName = "Name3"
+
+		if result, err := Client.UpdateOutgoingWebhook(hook); err != nil {
+			t.Fatal("Update hook should not fail")
+		} else {
+			updatedHook := result.Data.(*model.OutgoingWebhook)
+
+			if updatedHook.UpdateAt == hook.UpdateAt {
+				t.Fatal("failed - hook updateAt is not updated")
+			}
+		}
+	})
+
+	Client.Logout()
+	Client.Must(Client.LoginById(user2.Id, user2.Password))
+	Client.SetTeamId(team.Id)
+	t.Run("UpdateByUserWithoutPermissions", func(t *testing.T) {
+		if _, err := Client.UpdateOutgoingWebhook(hook); err == nil {
+			t.Fatal("should have failed - user does not have permissions to manage webhooks")
+		}
+
+		*utils.Cfg.ServiceSettings.EnableOnlyAdminIntegrations = false
+		utils.SetDefaultRolesBasedOnConfig()
+		t.Run("WithoutOnlyAdminIntegrations", func(t *testing.T) {
+			if _, err := Client.UpdateOutgoingWebhook(hook); err != nil {
+				t.Fatal("update webhook failed when admin only integrations is turned off")
+			}
+		})
+	})
+
+	*utils.Cfg.ServiceSettings.EnableOnlyAdminIntegrations = true
+	utils.SetDefaultRolesBasedOnConfig()
+
+	Client.Logout()
+	LinkUserToTeam(user3, team)
+	UpdateUserToTeamAdmin(user3, team)
+	Client.Must(Client.LoginById(user3.Id, user3.Password))
+	Client.SetTeamId(team.Id)
+	t.Run("RetainHookCreator", func(t *testing.T) {
+		if result, err := Client.UpdateOutgoingWebhook(hook); err != nil {
+			t.Fatal("failed to update outgoing web hook")
+		} else {
+			updatedHook := result.Data.(*model.OutgoingWebhook)
+
+			if updatedHook.CreatorId != user.Id {
+				t.Fatal("hook creator should not be changed")
+			}
+		}
+	})
+
+	Client.Logout()
+	Client.Must(Client.LoginById(user.Id, user.Password))
+	Client.SetTeamId(team.Id)
+	t.Run("UpdateToExistingTriggerWordAndCallback", func(t *testing.T) {
+		t.Run("OnSameChannel", func(t *testing.T) {
+			hook.TriggerWords = []string{"dogs"}
+
+			if _, err := Client.UpdateOutgoingWebhook(hook); err == nil {
+				t.Fatal("should have failed - duplicate trigger words & channel urls")
+			}
+		})
+
+		t.Run("OnDifferentChannel", func(t *testing.T) {
+			hook.TriggerWords = []string{"dogs"}
+			hook.ChannelId = channel3.Id
+
+			if _, err := Client.UpdateOutgoingWebhook(hook); err != nil {
+				t.Fatal("update of hook failed with duplicate trigger word but different channel")
+			}
+		})
+	})
+
+	t.Run("UpdateToNonExistentChannel", func(t *testing.T) {
+		hook.ChannelId = "junk"
+
+		if _, err := Client.UpdateOutgoingWebhook(hook); err == nil {
+			t.Fatal("should have failed - non existent channel")
+		}
+	})
+
+	t.Run("UpdateToPrivateChannel", func(t *testing.T) {
+		hook.ChannelId = channel2.Id
+
+		if _, err := Client.UpdateOutgoingWebhook(hook); err == nil {
+			t.Fatal("should have failed - update to a private channel")
+		}
+	})
+
+	t.Run("UpdateToBlankTriggerWordAndChannel", func(t *testing.T) {
+		hook.ChannelId = ""
+		hook.TriggerWords = nil
+
+		if _, err := Client.UpdateOutgoingWebhook(hook); err == nil {
+			t.Fatal("should have failed - update to blank trigger words & channel")
+		}
+	})
+
+	Client.Logout()
+	Client.Must(Client.LoginById(user3.Id, user3.Password))
+	Client.SetTeamId(team2.Id)
+	t.Run("UpdateToADifferentTeam", func(t *testing.T) {
+		if _, err := Client.UpdateOutgoingWebhook(hook); err == nil {
+			t.Fatal("should have failed - update to a different team is not allowed")
+		}
+	})
+}
+
 func TestDeleteOutgoingHook(t *testing.T) {
 	th := Setup().InitSystemAdmin()
 	Client := th.SystemAdminClient