MM-11228: Fix channel update/patch API endpoints. (#9073)

api4/channel.go

@@ -96,12 +96,28 @@ func updateChannel(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if _, err = c.App.GetChannelMember(channel.Id, c.Session.UserId); err != nil {
-		c.Err = err
-		return
-	}
+	switch oldChannel.Type {
+	case model.CHANNEL_OPEN:
+		if !c.App.SessionHasPermissionToChannel(c.Session, c.Params.ChannelId, model.PERMISSION_MANAGE_PUBLIC_CHANNEL_PROPERTIES) {
+			c.SetPermissionError(model.PERMISSION_MANAGE_PUBLIC_CHANNEL_PROPERTIES)
+			return
+		}
 
-	if !CanManageChannel(c, channel) {
+	case model.CHANNEL_PRIVATE:
+		if !c.App.SessionHasPermissionToChannel(c.Session, c.Params.ChannelId, model.PERMISSION_MANAGE_PRIVATE_CHANNEL_PROPERTIES) {
+			c.SetPermissionError(model.PERMISSION_MANAGE_PRIVATE_CHANNEL_PROPERTIES)
+			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.Session.UserId); err != nil {
+			c.Err = model.NewAppError("updateChannel", "api.channel.patch_update_channel.forbidden.app_error", nil, "", http.StatusForbidden)
+			return
+		}
+
+	default:
+		c.Err = model.NewAppError("updateChannel", "api.channel.patch_update_channel.forbidden.app_error", nil, "", http.StatusForbidden)
 		return
 	}
 
@@ -205,7 +221,28 @@ func patchChannel(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if !CanManageChannel(c, oldChannel) {
+	switch oldChannel.Type {
+	case model.CHANNEL_OPEN:
+		if !c.App.SessionHasPermissionToChannel(c.Session, c.Params.ChannelId, model.PERMISSION_MANAGE_PUBLIC_CHANNEL_PROPERTIES) {
+			c.SetPermissionError(model.PERMISSION_MANAGE_PUBLIC_CHANNEL_PROPERTIES)
+			return
+		}
+
+	case model.CHANNEL_PRIVATE:
+		if !c.App.SessionHasPermissionToChannel(c.Session, c.Params.ChannelId, model.PERMISSION_MANAGE_PRIVATE_CHANNEL_PROPERTIES) {
+			c.SetPermissionError(model.PERMISSION_MANAGE_PRIVATE_CHANNEL_PROPERTIES)
+			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.Session.UserId); err != nil {
+			c.Err = model.NewAppError("patchChannel", "api.channel.patch_update_channel.forbidden.app_error", nil, "", http.StatusForbidden)
+			return
+		}
+
+	default:
+		c.Err = model.NewAppError("patchChannel", "api.channel.patch_update_channel.forbidden.app_error", nil, "", http.StatusForbidden)
 		return
 	}
 
@@ -255,20 +292,6 @@ func restoreChannel(c *Context, w http.ResponseWriter, r *http.Request) {
 
 }
 
-func CanManageChannel(c *Context, channel *model.Channel) bool {
-	if channel.Type == model.CHANNEL_OPEN && !c.App.SessionHasPermissionToChannel(c.Session, channel.Id, model.PERMISSION_MANAGE_PUBLIC_CHANNEL_PROPERTIES) {
-		c.SetPermissionError(model.PERMISSION_MANAGE_PUBLIC_CHANNEL_PROPERTIES)
-		return false
-	}
-
-	if channel.Type == model.CHANNEL_PRIVATE && !c.App.SessionHasPermissionToChannel(c.Session, channel.Id, model.PERMISSION_MANAGE_PRIVATE_CHANNEL_PROPERTIES) {
-		c.SetPermissionError(model.PERMISSION_MANAGE_PRIVATE_CHANNEL_PROPERTIES)
-		return false
-	}
-
-	return true
-}
-
 func createDirectChannel(c *Context, w http.ResponseWriter, r *http.Request) {
 	userIds := model.ArrayFromJson(r.Body)
 	allowed := false

api4/channel_test.go

@@ -209,8 +209,34 @@ func TestUpdateChannel(t *testing.T) {
 
 	channel.DisplayName = "Should not update"
 	_, resp = Client.UpdateChannel(channel)
-	CheckNotFoundStatus(t, resp)
+	CheckForbiddenStatus(t, resp)
 
+	// Test updating the header of someone else's GM channel.
+	user1 := th.CreateUser()
+	user2 := th.CreateUser()
+	user3 := th.CreateUser()
+
+	groupChannel, resp := Client.CreateGroupChannel([]string{user1.Id, user2.Id})
+	CheckNoError(t, resp)
+
+	groupChannel.Header = "lolololol"
+	Client.Logout()
+	Client.Login(user3.Email, user3.Password)
+	_, resp = Client.UpdateChannel(groupChannel)
+	CheckForbiddenStatus(t, resp)
+
+	// Test updating the header of someone else's GM channel.
+	Client.Logout()
+	Client.Login(user.Email, user.Password)
+
+	directChannel, resp := Client.CreateDirectChannel(user.Id, user1.Id)
+	CheckNoError(t, resp)
+
+	directChannel.Header = "lolololol"
+	Client.Logout()
+	Client.Login(user3.Email, user3.Password)
+	_, resp = Client.UpdateChannel(directChannel)
+	CheckForbiddenStatus(t, resp)
 }
 
 func TestPatchChannel(t *testing.T) {
@@ -267,6 +293,36 @@ func TestPatchChannel(t *testing.T) {
 
 	_, resp = th.SystemAdminClient.PatchChannel(th.BasicPrivateChannel.Id, patch)
 	CheckNoError(t, resp)
+
+	// Test updating the header of someone else's GM channel.
+	user1 := th.CreateUser()
+	user2 := th.CreateUser()
+	user3 := th.CreateUser()
+
+	groupChannel, resp := Client.CreateGroupChannel([]string{user1.Id, user2.Id})
+	CheckNoError(t, resp)
+
+	Client.Logout()
+	Client.Login(user3.Email, user3.Password)
+
+	channelPatch := &model.ChannelPatch{}
+	channelPatch.Header = new(string)
+	*channelPatch.Header = "lolololol"
+
+	_, resp = Client.PatchChannel(groupChannel.Id, channelPatch)
+	CheckForbiddenStatus(t, resp)
+
+	// Test updating the header of someone else's GM channel.
+	Client.Logout()
+	Client.Login(user.Email, user.Password)
+
+	directChannel, resp := Client.CreateDirectChannel(user.Id, user1.Id)
+	CheckNoError(t, resp)
+
+	Client.Logout()
+	Client.Login(user3.Email, user3.Password)
+	_, resp = Client.PatchChannel(directChannel.Id, channelPatch)
+	CheckForbiddenStatus(t, resp)
 }
 
 func TestCreateDirectChannel(t *testing.T) {

i18n/en.json

@@ -87,6 +87,10 @@
     "id": "api.channel.add_user.to.channel.failed.app_error",
     "translation": "Failed to add user to channel"
   },
+  {
+    "id": "api.channel.patch_update_channel.forbidden.app_error",
+    "translation": "Failed to update the channel"
+  },
   {
     "id": "api.channel.add_user.to.channel.failed.deleted.app_error",
     "translation": "Failed to add user to channel because they have been removed from the team."