From 0bef611f7a2a89ceab05211c20f6a0c2758029ae Mon Sep 17 00:00:00 2001 From: George Goldberg Date: Tue, 2 Apr 2019 21:09:48 +0100 Subject: [PATCH] MM-14845: Fix Update and Patch channel for private channels. (#10539) * MM-14845: Fix Update and Patch channel for private channels. * Add additional failure case checks. --- api4/channel.go | 12 ++++++++++++ api4/channel_test.go | 13 ++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/api4/channel.go b/api4/channel.go index b490623951..b645a7de9c 100644 --- a/api4/channel.go +++ b/api4/channel.go @@ -119,6 +119,12 @@ func updateChannel(c *Context, w http.ResponseWriter, r *http.Request) { return } + // Since the `team_user` role can have PERMISSION_MANAGE_PRIVATE_CHANNEL_PROPERTIES out of the box, we must additionally check membership for private channels. + if _, memberErr := c.App.GetChannelMember(channel.Id, c.App.Session.UserId); memberErr != nil { + c.Err = model.NewAppError("updateChannel", "api.channel.patch_update_channel.forbidden.app_error", nil, "", http.StatusForbidden) + return + } + case model.CHANNEL_GROUP, model.CHANNEL_DIRECT: // Modifying the header is not linked to any specific permission for group/dm channels, so just check for membership. if _, err := c.App.GetChannelMember(channel.Id, c.App.Session.UserId); err != nil { @@ -252,6 +258,12 @@ func patchChannel(c *Context, w http.ResponseWriter, r *http.Request) { return } + // Since the `team_user` role can have PERMISSION_MANAGE_PRIVATE_CHANNEL_PROPERTIES out of the box, we must additionally check membership for private channels. + if _, memberErr := c.App.GetChannelMember(c.Params.ChannelId, c.App.Session.UserId); memberErr != nil { + c.Err = model.NewAppError("patchChannel", "api.channel.patch_update_channel.forbidden.app_error", nil, "", http.StatusForbidden) + return + } + case model.CHANNEL_GROUP, model.CHANNEL_DIRECT: // Modifying the header is not linked to any specific permission for group/dm channels, so just check for membership. if _, err = c.App.GetChannelMember(c.Params.ChannelId, c.App.Session.UserId); err != nil { diff --git a/api4/channel_test.go b/api4/channel_test.go index b55a5799a9..e70bf69f0a 100644 --- a/api4/channel_test.go +++ b/api4/channel_test.go @@ -193,6 +193,10 @@ func TestUpdateChannel(t *testing.T) { t.Fatal("Update failed for Purpose in private channel") } + // Updating a private channel requires permission *and* membership, so this should fail. + _, resp = th.SystemAdminClient.UpdateChannel(private) + CheckForbiddenStatus(t, resp) + //Non existing channel channel1 := &model.Channel{DisplayName: "Test API Name for apiv4", Name: GenerateTestChannelName(), Type: model.CHANNEL_OPEN, TeamId: team.Id} _, resp = Client.UpdateChannel(channel1) @@ -291,9 +295,16 @@ func TestPatchChannel(t *testing.T) { _, resp = th.SystemAdminClient.PatchChannel(th.BasicChannel.Id, patch) CheckNoError(t, resp) - _, resp = th.SystemAdminClient.PatchChannel(th.BasicPrivateChannel.Id, patch) + Client.Logout() + Client.Login(th.BasicUser.Username, th.BasicUser.Password) + + _, resp = th.Client.PatchChannel(th.BasicPrivateChannel.Id, patch) CheckNoError(t, resp) + // Patching a private channel requires permission *and* membership, so this should fail. + _, resp = th.SystemAdminClient.PatchChannel(th.BasicPrivateChannel.Id, patch) + CheckForbiddenStatus(t, resp) + // Test updating the header of someone else's GM channel. user1 := th.CreateUser() user2 := th.CreateUser()