[release-10.11] Restrict group_constrained to channels that support group sync (#36916)

Automatic Merge

server/channels/api4/channel.go

@@ -345,6 +345,11 @@ func patchChannel(c *Context, w http.ResponseWriter, r *http.Request) {
 	model.AddEventParameterAuditableToAuditRec(auditRec, "channel", patch)
 	auditRec.AddEventPriorState(oldChannel)
 
+	if patch.GroupConstrained != nil && !oldChannel.SupportsGroupSync() {
+		c.Err = model.NewAppError("patchChannel", "api.channel.patch_update_channel.group_constrained_not_allowed.app_error", nil, "", http.StatusBadRequest)
+		return
+	}
+
 	switch oldChannel.Type {
 	case model.ChannelTypeOpen:
 		if ok, _ := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), c.Params.ChannelId, model.PermissionManagePublicChannelProperties); !ok {

server/channels/api4/channel_test.go

@@ -872,6 +872,36 @@ func TestPatchChannel(t *testing.T) {
 		CheckBadRequestStatus(t, resp)
 	})
 
+	t.Run("Should block setting group_constrained on group and direct messages", func(t *testing.T) {
+		user1 := th.CreateUser()
+		user2 := th.CreateUser()
+		user3 := th.CreateUser()
+
+		_, err := client.Logout(context.Background())
+		require.NoError(t, err)
+		_, _, err = client.Login(context.Background(), user1.Email, user1.Password)
+		require.NoError(t, err)
+
+		groupChannel, _, err := client.CreateGroupChannel(context.Background(), []string{user1.Id, user2.Id, user3.Id})
+		require.NoError(t, err)
+
+		patch := &model.ChannelPatch{GroupConstrained: model.NewPointer(true)}
+		_, resp, err := client.PatchChannel(context.Background(), groupChannel.Id, patch)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, resp)
+
+		stats, _, err := client.GetChannelStats(context.Background(), groupChannel.Id, "", false)
+		require.NoError(t, err)
+		require.Equal(t, int64(3), stats.MemberCount)
+
+		directChannel, _, err := client.CreateDirectChannel(context.Background(), user1.Id, user2.Id)
+		require.NoError(t, err)
+
+		_, resp, err = client.PatchChannel(context.Background(), directChannel.Id, patch)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, resp)
+	})
+
 	t.Run("Should not be able to configure channel banner without a license", func(t *testing.T) {
 		_, err := client.Logout(context.Background())
 		require.NoError(t, err)

server/channels/store/sqlstore/group_store.go

@@ -1268,6 +1268,8 @@ func (s *SqlGroupStore) ChannelMembersToRemove(channelID *string) ([]*model.Chan
 		Join("Channels ON Channels.Id = ChannelMembers.ChannelId").
 		LeftJoin("Bots ON Bots.UserId = ChannelMembers.UserId").
 		Where(sq.Eq{"Channels.DeleteAt": 0, "Channels.GroupConstrained": true, "Bots.UserId": nil}).
+		// Only public/private channels support group sync; never treat other channel members as removable.
+		Where(sq.Eq{"Channels.Type": []model.ChannelType{model.ChannelTypeOpen, model.ChannelTypePrivate}}).
 		Where(whereStmt)
 
 	if channelID != nil {

server/i18n/en.json

@@ -455,6 +455,10 @@
     "id": "api.channel.patch_update_channel.forbidden.app_error",
     "translation": "Failed to update the channel."
   },
+  {
+    "id": "api.channel.patch_update_channel.group_constrained_not_allowed.app_error",
+    "translation": "You are not allowed to set group_constrained on this channel type."
+  },
   {
     "id": "api.channel.patch_update_channel.update_direct_or_group_messages_not_allowed.app_error",
     "translation": "You are not allowed to update the name, display_name, and purpose of direct or group messages."
@@ -8980,6 +8984,10 @@
     "id": "model.channel.is_valid.display_name.app_error",
     "translation": "Invalid display name."
   },
+  {
+    "id": "model.channel.is_valid.group_constrained.app_error",
+    "translation": "Only public and private channels can be group constrained."
+  },
   {
     "id": "model.channel.is_valid.header.app_error",
     "translation": "Invalid header."

server/public/model/channel.go

@@ -329,6 +329,10 @@ func (o *Channel) IsValid() *AppError {
 		}
 	}
 
+	if o.IsGroupConstrained() && !o.SupportsGroupSync() {
+		return NewAppError("Channel.IsValid", "model.channel.is_valid.group_constrained.app_error", nil, "id="+o.Id, http.StatusBadRequest)
+	}
+
 	return nil
 }
 
@@ -356,6 +360,11 @@ func (o *Channel) IsGroupOrDirect() bool {
 	return o.Type == ChannelTypeDirect || o.Type == ChannelTypeGroup
 }
 
+// SupportsGroupSync reports whether group_constrained is meaningful for the channel type.
+func (o *Channel) SupportsGroupSync() bool {
+	return o.Type == ChannelTypeOpen || o.Type == ChannelTypePrivate
+}
+
 func (o *Channel) IsOpen() bool {
 	return o.Type == ChannelTypeOpen
 }

server/public/model/channel_test.go

@@ -35,6 +35,47 @@ func TestChannelPatch(t *testing.T) {
 	require.Equal(t, *p.GroupConstrained, *o.GroupConstrained)
 }
 
+func TestChannelSupportsGroupSync(t *testing.T) {
+	require.True(t, (&Channel{Type: ChannelTypeOpen}).SupportsGroupSync())
+	require.True(t, (&Channel{Type: ChannelTypePrivate}).SupportsGroupSync())
+	require.False(t, (&Channel{Type: ChannelTypeDirect}).SupportsGroupSync())
+	require.False(t, (&Channel{Type: ChannelTypeGroup}).SupportsGroupSync())
+}
+
+func TestChannelIsValidGroupConstrained(t *testing.T) {
+	base := Channel{
+		Id:          NewId(),
+		CreateAt:    GetMillis(),
+		UpdateAt:    GetMillis(),
+		DisplayName: "x",
+		Name:        "valid-name",
+		Header:      "h",
+		Purpose:     "p",
+	}
+
+	t.Run("group_constrained is allowed on public and private channels", func(t *testing.T) {
+		c := base
+		c.GroupConstrained = NewPointer(true)
+
+		c.Type = ChannelTypeOpen
+		require.Nil(t, c.IsValid())
+
+		c.Type = ChannelTypePrivate
+		require.Nil(t, c.IsValid())
+	})
+
+	t.Run("group_constrained is rejected on direct and group channels", func(t *testing.T) {
+		c := base
+		c.GroupConstrained = NewPointer(true)
+
+		c.Type = ChannelTypeDirect
+		require.NotNil(t, c.IsValid())
+
+		c.Type = ChannelTypeGroup
+		require.NotNil(t, c.IsValid())
+	})
+}
+
 func TestChannelIsValid(t *testing.T) {
 	o := Channel{}