[MM-16697] Rework validation of team and channel membership for webhook updates (#11483)

* Validate team and channel membership for webhook updates

* Implerment review feedback

* Readability

* Additional controls for team id comparison

api4/webhook.go

@@ -96,23 +96,28 @@ func updateIncomingHook(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if !c.App.SessionHasPermissionToTeam(c.App.Session, updatedHook.TeamId, model.PERMISSION_MANAGE_INCOMING_WEBHOOKS) {
+	channel, err := c.App.GetChannel(updatedHook.ChannelId)
+	if err != nil {
+		c.Err = err
+		return
+	}
+
+	if channel.TeamId != updatedHook.TeamId {
+		c.SetInvalidParam("channel_id")
+		return
+	}
+
+	if !c.App.SessionHasPermissionToTeam(c.App.Session, channel.TeamId, model.PERMISSION_MANAGE_INCOMING_WEBHOOKS) {
 		c.SetPermissionError(model.PERMISSION_MANAGE_INCOMING_WEBHOOKS)
 		return
 	}
 
-	if c.App.Session.UserId != oldHook.UserId && !c.App.SessionHasPermissionToTeam(c.App.Session, updatedHook.TeamId, model.PERMISSION_MANAGE_OTHERS_INCOMING_WEBHOOKS) {
+	if c.App.Session.UserId != oldHook.UserId && !c.App.SessionHasPermissionToTeam(c.App.Session, channel.TeamId, model.PERMISSION_MANAGE_OTHERS_INCOMING_WEBHOOKS) {
 		c.LogAudit("fail - inappropriate permissions")
 		c.SetPermissionError(model.PERMISSION_MANAGE_OTHERS_INCOMING_WEBHOOKS)
 		return
 	}
 
-	channel, err := c.App.GetChannel(updatedHook.ChannelId)
-	if err != nil {
-		c.Err = err
-		return
-	}
-
 	if channel.Type != model.CHANNEL_OPEN && !c.App.SessionHasPermissionToChannel(c.App.Session, channel.Id, model.PERMISSION_READ_CHANNEL) {
 		c.LogAudit("fail - bad channel permissions")
 		c.SetPermissionError(model.PERMISSION_READ_CHANNEL)

api4/webhook_test.go

@@ -74,6 +74,41 @@ func TestCreateIncomingWebhook(t *testing.T) {
 	CheckNotImplementedStatus(t, resp)
 }
 
+
+func TestCreateIncomingWebhook_BypassTeamPermissions(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableIncomingWebhooks = true })
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnablePostUsernameOverride = true })
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnablePostIconOverride = true })
+
+	defaultRolePermissions := th.SaveDefaultRolePermissions()
+	defer th.RestoreDefaultRolePermissions(defaultRolePermissions)
+	th.RemovePermissionFromRole(model.PERMISSION_MANAGE_INCOMING_WEBHOOKS.Id, model.SYSTEM_USER_ROLE_ID)
+	th.AddPermissionToRole(model.PERMISSION_MANAGE_INCOMING_WEBHOOKS.Id, model.TEAM_ADMIN_ROLE_ID)
+	th.AddPermissionToRole(model.PERMISSION_MANAGE_INCOMING_WEBHOOKS.Id, model.TEAM_USER_ROLE_ID)
+
+	hook := &model.IncomingWebhook{ChannelId: th.BasicChannel.Id}
+
+	rhook, resp := th.Client.CreateIncomingWebhook(hook)
+	CheckNoError(t, resp)
+
+	require.Equal(t, rhook.ChannelId, hook.ChannelId)
+	require.Equal(t, rhook.UserId, th.BasicUser.Id)
+	require.Equal(t, rhook.TeamId,th.BasicTeam.Id)
+
+	team := th.CreateTeam()
+	team.AllowOpenInvite = false
+	th.Client.UpdateTeam(team)
+	th.SystemAdminClient.RemoveTeamMember(team.Id, th.BasicUser.Id)
+	channel := th.CreateChannelWithClientAndTeam(th.SystemAdminClient, model.CHANNEL_OPEN, team.Id)
+
+	hook = &model.IncomingWebhook{ChannelId: channel.Id}
+	rhook, resp = th.Client.CreateIncomingWebhook(hook)
+	CheckForbiddenStatus(t, resp)
+}
+
 func TestGetIncomingWebhooks(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
@@ -639,6 +674,40 @@ func TestUpdateIncomingHook(t *testing.T) {
 	})
 }
 
+func TestUpdateIncomingWebhook_BypassTeamPermissions(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableIncomingWebhooks = true })
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnablePostUsernameOverride = true })
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnablePostIconOverride = true })
+
+	defaultRolePermissions := th.SaveDefaultRolePermissions()
+	defer th.RestoreDefaultRolePermissions(defaultRolePermissions)
+	th.RemovePermissionFromRole(model.PERMISSION_MANAGE_INCOMING_WEBHOOKS.Id, model.SYSTEM_USER_ROLE_ID)
+	th.AddPermissionToRole(model.PERMISSION_MANAGE_INCOMING_WEBHOOKS.Id, model.TEAM_ADMIN_ROLE_ID)
+	th.AddPermissionToRole(model.PERMISSION_MANAGE_INCOMING_WEBHOOKS.Id, model.TEAM_USER_ROLE_ID)
+
+	hook := &model.IncomingWebhook{ChannelId: th.BasicChannel.Id}
+
+	rhook, resp := th.Client.CreateIncomingWebhook(hook)
+	CheckNoError(t, resp)
+
+	require.Equal(t, rhook.ChannelId, hook.ChannelId)
+	require.Equal(t, rhook.UserId, th.BasicUser.Id)
+	require.Equal(t, rhook.TeamId,th.BasicTeam.Id)
+
+	team := th.CreateTeam()
+	team.AllowOpenInvite = false
+	th.Client.UpdateTeam(team)
+	th.SystemAdminClient.RemoveTeamMember(team.Id, th.BasicUser.Id)
+	channel := th.CreateChannelWithClientAndTeam(th.SystemAdminClient, model.CHANNEL_OPEN, team.Id)
+
+	hook2 := &model.IncomingWebhook{Id: rhook.Id, ChannelId: channel.Id}
+	rhook, resp = th.Client.UpdateIncomingWebhook(hook2)
+	CheckBadRequestStatus(t, resp)
+}
+
 func TestRegenOutgoingHookToken(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
@@ -834,6 +903,38 @@ func TestUpdateOutgoingHook(t *testing.T) {
 	})
 }
 
+func TestUpdateOutgoingWebhook_BypassTeamPermissions(t *testing.T) {
+	th := Setup().InitBasic()
+	defer th.TearDown()
+
+	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableOutgoingWebhooks = true })
+
+	defaultRolePermissions := th.SaveDefaultRolePermissions()
+	defer th.RestoreDefaultRolePermissions(defaultRolePermissions)
+	th.RemovePermissionFromRole(model.PERMISSION_MANAGE_OUTGOING_WEBHOOKS.Id, model.SYSTEM_USER_ROLE_ID)
+	th.AddPermissionToRole(model.PERMISSION_MANAGE_OUTGOING_WEBHOOKS.Id, model.TEAM_ADMIN_ROLE_ID)
+	th.AddPermissionToRole(model.PERMISSION_MANAGE_OUTGOING_WEBHOOKS.Id, model.TEAM_USER_ROLE_ID)
+
+	hook := &model.OutgoingWebhook{ChannelId: th.BasicChannel.Id, TeamId: th.BasicChannel.TeamId,
+		CallbackURLs: []string{"http://nowhere.com"}, TriggerWords: []string{"rats2"}}
+
+	rhook, resp := th.Client.CreateOutgoingWebhook(hook)
+	CheckNoError(t, resp)
+
+	require.Equal(t, rhook.ChannelId, hook.ChannelId)
+	require.Equal(t, rhook.TeamId,th.BasicTeam.Id)
+
+	team := th.CreateTeam()
+	team.AllowOpenInvite = false
+	th.Client.UpdateTeam(team)
+	th.SystemAdminClient.RemoveTeamMember(team.Id, th.BasicUser.Id)
+	channel := th.CreateChannelWithClientAndTeam(th.SystemAdminClient, model.CHANNEL_OPEN, team.Id)
+
+	hook2 := &model.OutgoingWebhook{Id: rhook.Id, ChannelId: channel.Id}
+	rhook, resp = th.Client.UpdateOutgoingWebhook(hook2)
+	CheckForbiddenStatus(t, resp)
+}
+
 func TestDeleteOutgoingHook(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()