MM-68547: Tighten authorization on group syncable link and patch endpoints (#36316) (#36431)

Automatic Merge

e2e-tests/cypress/tests/integration/channels/enterprise/system_console/group_configuration_spec.js

@@ -394,8 +394,10 @@ describe('group configuration', () => {
             // # Save settings
             savePage();
 
-            // * Check the groupteam via the API to ensure its role wasn't updated
+            // * Check the groupteam via the API to ensure the team was
+            // removed (delete_at != 0) and its role wasn't updated.
             cy.apiGetGroupTeam(groupID, testTeam.id).then(({body}) => {
+                expect(body.delete_at).to.not.eq(0);
                 expect(body.scheme_admin).to.eq(false);
             });
         });
@@ -519,8 +521,10 @@ describe('group configuration', () => {
             // # Save settings
             savePage();
 
-            // * Check the groupteam via the API to ensure its role wasn't updated
+            // * Check the groupteam via the API to ensure the channel was
+            // removed (delete_at != 0) and its role wasn't updated.
             cy.apiGetGroupChannel(groupID, testChannel.id).then(({body}) => {
+                expect(body.delete_at).to.not.eq(0);
                 expect(body.scheme_admin).to.eq(false);
             });
         });

server/channels/api4/group.go

@@ -372,10 +372,35 @@ func linkGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	groupSyncable := &model.GroupSyncable{
-		GroupId:    c.Params.GroupId,
-		SyncableId: syncableID,
-		Type:       syncableType,
+	appErr = verifySchemeAdminAssignmentPermission(c, syncableType, syncableID, patch)
+	if appErr != nil {
+		appErr.Where = "Api4.linkGroupSyncable"
+		c.Err = appErr
+		return
+	}
+
+	// Upsert onto the existing row only when it is currently active so
+	// unspecified fields are preserved. A fresh link, or a re-link of a
+	// soft-deleted row, starts from a zero-value struct so that fields
+	// the caller did not (or was not authorized to) set are not carried
+	// over from the previous incarnation. The downstream upsert clears
+	// DeleteAt when re-activating.
+	existing, appErr := c.App.GetGroupSyncable(c.Params.GroupId, syncableID, syncableType)
+	if appErr != nil && appErr.StatusCode != http.StatusNotFound {
+		appErr.Where = "Api4.linkGroupSyncable"
+		c.Err = appErr
+		return
+	}
+
+	var groupSyncable *model.GroupSyncable
+	if existing != nil && existing.DeleteAt == 0 {
+		groupSyncable = existing
+	} else {
+		groupSyncable = &model.GroupSyncable{
+			GroupId:    c.Params.GroupId,
+			SyncableId: syncableID,
+			Type:       syncableType,
+		}
 	}
 	groupSyncable.Patch(patch)
 	groupSyncable, appErr = c.App.UpsertGroupSyncable(groupSyncable)
@@ -387,8 +412,9 @@ func linkGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) {
 	auditRec.AddEventResultState(groupSyncable)
 	auditRec.AddEventObjectType("group_syncable")
 
+	syncRoles := patch.SchemeAdmin != nil
 	c.App.Srv().Go(func() {
-		c.App.SyncRolesAndMembership(c.AppContext, syncableID, syncableType, c.Params.GroupId)
+		c.App.SyncRolesAndMembership(c.AppContext, syncableID, syncableType, c.Params.GroupId, syncRoles)
 	})
 
 	w.WriteHeader(http.StatusCreated)
@@ -555,6 +581,13 @@ func patchGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	appErr = verifySchemeAdminAssignmentPermission(c, syncableType, syncableID, patch)
+	if appErr != nil {
+		appErr.Where = "Api4.patchGroupSyncable"
+		c.Err = appErr
+		return
+	}
+
 	groupSyncable, appErr := c.App.GetGroupSyncable(c.Params.GroupId, syncableID, syncableType)
 	if appErr != nil {
 		c.Err = appErr
@@ -572,8 +605,9 @@ func patchGroupSyncable(c *Context, w http.ResponseWriter, r *http.Request) {
 	auditRec.AddEventResultState(groupSyncable)
 	auditRec.AddEventObjectType("group_syncable")
 
+	syncRoles := patch.SchemeAdmin != nil
 	c.App.Srv().Go(func() {
-		c.App.SyncRolesAndMembership(c.AppContext, syncableID, syncableType, c.Params.GroupId)
+		c.App.SyncRolesAndMembership(c.AppContext, syncableID, syncableType, c.Params.GroupId, syncRoles)
 	})
 
 	b, err := json.Marshal(groupSyncable)
@@ -705,6 +739,34 @@ func verifyLinkUnlinkPermission(c *Context, syncableType model.GroupSyncableType
 	return nil
 }
 
+// verifySchemeAdminAssignmentPermission requires the caller to hold the
+// role-management permission for the target syncable
+// (manage_team_roles / manage_channel_roles), or the sysconsole groups
+// write permission, before an explicit SchemeAdmin value in the patch is
+// accepted. A nil patch.SchemeAdmin is a no-op.
+func verifySchemeAdminAssignmentPermission(c *Context, syncableType model.GroupSyncableType, syncableID string, patch *model.GroupSyncablePatch) *model.AppError {
+	if patch == nil || patch.SchemeAdmin == nil {
+		return nil
+	}
+
+	if c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleWriteUserManagementGroups) {
+		return nil
+	}
+
+	switch syncableType {
+	case model.GroupSyncableTypeTeam:
+		if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), syncableID, model.PermissionManageTeamRoles) {
+			return model.MakePermissionError(c.AppContext.Session(), []*model.Permission{model.PermissionManageTeamRoles})
+		}
+	case model.GroupSyncableTypeChannel:
+		if ok, _ := c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), syncableID, model.PermissionManageChannelRoles); !ok {
+			return model.MakePermissionError(c.AppContext.Session(), []*model.Permission{model.PermissionManageChannelRoles})
+		}
+	}
+
+	return nil
+}
+
 func getGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) {
 	permissionErr := requireLicense(c)
 	if permissionErr != nil {

server/channels/api4/group_test.go

@@ -3002,3 +3002,736 @@ func TestDeleteMembersFromGroup(t *testing.T) {
 		CheckBadRequestStatus(t, response)
 	})
 }
+
+// newSchemeAdminTestLdapGroup creates a fresh LDAP-source group with
+// AllowReference=true.
+func newSchemeAdminTestLdapGroup(t *testing.T, th *TestHelper) *model.Group {
+	t.Helper()
+	id := model.NewId()
+	g, appErr := th.App.CreateGroup(&model.Group{
+		DisplayName:    "dn_" + id,
+		Name:           model.NewPointer("name" + id),
+		Source:         model.GroupSourceLdap,
+		Description:    "description_" + id,
+		RemoteId:       model.NewPointer(model.NewId()),
+		AllowReference: true,
+	})
+	require.Nil(t, appErr)
+	return g
+}
+
+// findPersistedGroupSyncable returns the persisted GroupSyncable for a
+// given (groupID, syncableID, syncableType) tuple, including SchemeAdmin.
+func findPersistedGroupSyncable(t *testing.T, th *TestHelper, groupID, syncableID string, syncableType model.GroupSyncableType) *model.GroupSyncable {
+	t.Helper()
+	syncables, appErr := th.App.GetGroupSyncables(groupID, syncableType)
+	require.Nil(t, appErr)
+	for _, s := range syncables {
+		if s.SyncableId == syncableID {
+			return s
+		}
+	}
+	return nil
+}
+
+func TestLinkGroupTeam_SchemeAdminRequiresElevatedPermission(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+
+	schemeAdminTrue := &model.GroupSyncablePatch{
+		AutoAdd:     model.NewPointer(true),
+		SchemeAdmin: model.NewPointer(true),
+	}
+
+	t.Run("regular team user with invite_user must NOT be able to set scheme_admin: true", func(t *testing.T) {
+		g := newSchemeAdminTestLdapGroup(t, th)
+
+		groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, response)
+		assert.Nil(t, groupSyncable)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		if persisted != nil {
+			assert.False(t, persisted.SchemeAdmin)
+		}
+	})
+
+	t.Run("system admin can still set scheme_admin: true", func(t *testing.T) {
+		g := newSchemeAdminTestLdapGroup(t, th)
+
+		_, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue)
+		require.NoError(t, err)
+		CheckCreatedStatus(t, response)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NotNil(t, persisted)
+		assert.True(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("regular team user can still link with scheme_admin omitted", func(t *testing.T) {
+		g := newSchemeAdminTestLdapGroup(t, th)
+
+		patch := &model.GroupSyncablePatch{
+			AutoAdd: model.NewPointer(true),
+		}
+		groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch)
+		require.NoError(t, err)
+		CheckCreatedStatus(t, response)
+		require.NotNil(t, groupSyncable)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NotNil(t, persisted)
+		assert.False(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("regular team user must NOT be able to link with scheme_admin: false explicitly", func(t *testing.T) {
+		g := newSchemeAdminTestLdapGroup(t, th)
+
+		patch := &model.GroupSyncablePatch{
+			AutoAdd:     model.NewPointer(true),
+			SchemeAdmin: model.NewPointer(false),
+		}
+		groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, response)
+		assert.Nil(t, groupSyncable)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		if persisted != nil {
+			assert.False(t, persisted.SchemeAdmin)
+		}
+	})
+}
+
+func TestLinkGroupChannel_SchemeAdminRequiresElevatedPermission(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+
+	// A regular user can only link a channel syncable when the group is
+	// already linked to the parent team, so seed the team link as sysadmin.
+	mkLinkedGroup := func(t *testing.T) *model.Group {
+		t.Helper()
+		g := newSchemeAdminTestLdapGroup(t, th)
+		_, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{
+			AutoAdd: model.NewPointer(true),
+		})
+		require.NoError(t, err)
+		CheckCreatedStatus(t, response)
+		return g
+	}
+
+	schemeAdminTrue := &model.GroupSyncablePatch{
+		AutoAdd:     model.NewPointer(true),
+		SchemeAdmin: model.NewPointer(true),
+	}
+
+	t.Run("regular channel user with manage_*_channel_members must NOT be able to set scheme_admin: true", func(t *testing.T) {
+		g := mkLinkedGroup(t)
+
+		groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, response)
+		assert.Nil(t, groupSyncable)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		if persisted != nil {
+			assert.False(t, persisted.SchemeAdmin)
+		}
+	})
+
+	t.Run("system admin can still set scheme_admin: true", func(t *testing.T) {
+		g := mkLinkedGroup(t)
+
+		_, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue)
+		require.NoError(t, err)
+		CheckCreatedStatus(t, response)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		require.NotNil(t, persisted)
+		assert.True(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("regular channel user can still link with scheme_admin omitted", func(t *testing.T) {
+		g := mkLinkedGroup(t)
+
+		patch := &model.GroupSyncablePatch{
+			AutoAdd: model.NewPointer(true),
+		}
+		groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch)
+		require.NoError(t, err)
+		CheckCreatedStatus(t, response)
+		require.NotNil(t, groupSyncable)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		require.NotNil(t, persisted)
+		assert.False(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("regular channel user must NOT be able to link with scheme_admin: false explicitly", func(t *testing.T) {
+		g := mkLinkedGroup(t)
+
+		patch := &model.GroupSyncablePatch{
+			AutoAdd:     model.NewPointer(true),
+			SchemeAdmin: model.NewPointer(false),
+		}
+		groupSyncable, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, response)
+		assert.Nil(t, groupSyncable)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		if persisted != nil {
+			assert.False(t, persisted.SchemeAdmin)
+		}
+	})
+}
+
+func TestPatchGroupTeam_SchemeAdminRequiresElevatedPermission(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+
+	// schemeAdmin controls the seeded SchemeAdmin value on the team syncable.
+	setupLinkedGroup := func(t *testing.T, schemeAdmin bool) *model.Group {
+		t.Helper()
+		g := newSchemeAdminTestLdapGroup(t, th)
+		_, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{
+			AutoAdd:     model.NewPointer(true),
+			SchemeAdmin: model.NewPointer(schemeAdmin),
+		})
+		require.NoError(t, err)
+		CheckCreatedStatus(t, response)
+		return g
+	}
+
+	schemeAdminTrue := &model.GroupSyncablePatch{
+		SchemeAdmin: model.NewPointer(true),
+	}
+
+	schemeAdminFalse := &model.GroupSyncablePatch{
+		SchemeAdmin: model.NewPointer(false),
+	}
+
+	t.Run("regular team user with invite_user must NOT be able to patch scheme_admin: true", func(t *testing.T) {
+		g := setupLinkedGroup(t, false)
+
+		_, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, response)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NotNil(t, persisted)
+		assert.False(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("system admin can still patch scheme_admin: true", func(t *testing.T) {
+		g := setupLinkedGroup(t, false)
+
+		_, response, err := th.SystemAdminClient.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NotNil(t, persisted)
+		assert.True(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("regular team user can still patch other fields with scheme_admin omitted", func(t *testing.T) {
+		g := setupLinkedGroup(t, true)
+
+		patch := &model.GroupSyncablePatch{
+			AutoAdd: model.NewPointer(false),
+		}
+		_, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NotNil(t, persisted)
+		assert.False(t, persisted.AutoAdd)
+		assert.True(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("regular team user must NOT be able to patch scheme_admin: false", func(t *testing.T) {
+		g := setupLinkedGroup(t, true)
+
+		_, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminFalse)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, response)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NotNil(t, persisted)
+		assert.True(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("system admin can still patch scheme_admin: false", func(t *testing.T) {
+		g := setupLinkedGroup(t, true)
+
+		_, response, err := th.SystemAdminClient.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminFalse)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NotNil(t, persisted)
+		assert.False(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("sysconsole_write_user_management_groups holder can patch scheme_admin in either direction", func(t *testing.T) {
+		// system_manager bundles sysconsole_write_user_management_groups,
+		// the override honoured by verifySchemeAdminAssignmentPermission.
+		th.LoginSystemManager(t)
+
+		gPromote := setupLinkedGroup(t, false)
+		_, response, err := th.SystemManagerClient.PatchGroupSyncable(context.Background(), gPromote.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminTrue)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+		persistedPromote := findPersistedGroupSyncable(t, th, gPromote.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NotNil(t, persistedPromote)
+		assert.True(t, persistedPromote.SchemeAdmin)
+
+		gDemote := setupLinkedGroup(t, true)
+		_, response, err = th.SystemManagerClient.PatchGroupSyncable(context.Background(), gDemote.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, schemeAdminFalse)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+		persistedDemote := findPersistedGroupSyncable(t, th, gDemote.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NotNil(t, persistedDemote)
+		assert.False(t, persistedDemote.SchemeAdmin)
+	})
+}
+
+func TestPatchGroupChannel_SchemeAdminRequiresElevatedPermission(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+
+	// schemeAdmin controls the seeded SchemeAdmin value on the channel
+	// syncable. The team syncable is seeded so the channel link succeeds.
+	setupLinkedGroup := func(t *testing.T, schemeAdmin bool) *model.Group {
+		t.Helper()
+		g := newSchemeAdminTestLdapGroup(t, th)
+		_, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{
+			AutoAdd: model.NewPointer(true),
+		})
+		require.NoError(t, err)
+		CheckCreatedStatus(t, response)
+
+		_, response, err = th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, &model.GroupSyncablePatch{
+			AutoAdd:     model.NewPointer(true),
+			SchemeAdmin: model.NewPointer(schemeAdmin),
+		})
+		require.NoError(t, err)
+		CheckCreatedStatus(t, response)
+		return g
+	}
+
+	schemeAdminTrue := &model.GroupSyncablePatch{
+		SchemeAdmin: model.NewPointer(true),
+	}
+
+	schemeAdminFalse := &model.GroupSyncablePatch{
+		SchemeAdmin: model.NewPointer(false),
+	}
+
+	t.Run("regular channel user with manage_*_channel_members must NOT be able to patch scheme_admin: true", func(t *testing.T) {
+		g := setupLinkedGroup(t, false)
+
+		_, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, response)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		require.NotNil(t, persisted)
+		assert.False(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("system admin can still patch scheme_admin: true", func(t *testing.T) {
+		g := setupLinkedGroup(t, false)
+
+		_, response, err := th.SystemAdminClient.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		require.NotNil(t, persisted)
+		assert.True(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("regular channel user can still patch other fields with scheme_admin omitted", func(t *testing.T) {
+		g := setupLinkedGroup(t, true)
+
+		patch := &model.GroupSyncablePatch{
+			AutoAdd: model.NewPointer(false),
+		}
+		_, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		require.NotNil(t, persisted)
+		assert.False(t, persisted.AutoAdd)
+		assert.True(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("regular channel user must NOT be able to patch scheme_admin: false", func(t *testing.T) {
+		g := setupLinkedGroup(t, true)
+
+		_, response, err := th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminFalse)
+		require.Error(t, err)
+		CheckForbiddenStatus(t, response)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		require.NotNil(t, persisted)
+		assert.True(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("system admin can still patch scheme_admin: false", func(t *testing.T) {
+		g := setupLinkedGroup(t, true)
+
+		_, response, err := th.SystemAdminClient.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminFalse)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		require.NotNil(t, persisted)
+		assert.False(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("sysconsole_write_user_management_groups holder can patch scheme_admin in either direction", func(t *testing.T) {
+		// system_manager bundles sysconsole_write_user_management_groups,
+		// the override honoured by verifySchemeAdminAssignmentPermission.
+		th.LoginSystemManager(t)
+
+		gPromote := setupLinkedGroup(t, false)
+		_, response, err := th.SystemManagerClient.PatchGroupSyncable(context.Background(), gPromote.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminTrue)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+		persistedPromote := findPersistedGroupSyncable(t, th, gPromote.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		require.NotNil(t, persistedPromote)
+		assert.True(t, persistedPromote.SchemeAdmin)
+
+		gDemote := setupLinkedGroup(t, true)
+		_, response, err = th.SystemManagerClient.PatchGroupSyncable(context.Background(), gDemote.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, schemeAdminFalse)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+		persistedDemote := findPersistedGroupSyncable(t, th, gDemote.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		require.NotNil(t, persistedDemote)
+		assert.False(t, persistedDemote.SchemeAdmin)
+	})
+}
+
+func TestLinkGroupTeam_LinkOnExistingPreservesSchemeAdmin(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+
+	seedSchemeAdminTrue := func(t *testing.T) *model.Group {
+		t.Helper()
+		g := newSchemeAdminTestLdapGroup(t, th)
+		_, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{
+			AutoAdd:     model.NewPointer(true),
+			SchemeAdmin: model.NewPointer(true),
+		})
+		require.NoError(t, err)
+		CheckCreatedStatus(t, response)
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NotNil(t, persisted)
+		require.True(t, persisted.SchemeAdmin)
+		return g
+	}
+
+	t.Run("regular team user calling LINK with scheme_admin omitted must not change persisted scheme_admin", func(t *testing.T) {
+		g := seedSchemeAdminTrue(t)
+
+		patch := &model.GroupSyncablePatch{
+			AutoAdd: model.NewPointer(true),
+		}
+		_, _, _ = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NotNil(t, persisted)
+		assert.True(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("regular team user calling LINK with scheme_admin: false must not change persisted scheme_admin", func(t *testing.T) {
+		g := seedSchemeAdminTrue(t)
+
+		patch := &model.GroupSyncablePatch{
+			AutoAdd:     model.NewPointer(true),
+			SchemeAdmin: model.NewPointer(false),
+		}
+		_, _, _ = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NotNil(t, persisted)
+		assert.True(t, persisted.SchemeAdmin)
+	})
+}
+
+func TestLinkGroupChannel_LinkOnExistingPreservesSchemeAdmin(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+
+	seedSchemeAdminTrue := func(t *testing.T) *model.Group {
+		t.Helper()
+		g := newSchemeAdminTestLdapGroup(t, th)
+		_, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{
+			AutoAdd: model.NewPointer(true),
+		})
+		require.NoError(t, err)
+		CheckCreatedStatus(t, response)
+
+		_, response, err = th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, &model.GroupSyncablePatch{
+			AutoAdd:     model.NewPointer(true),
+			SchemeAdmin: model.NewPointer(true),
+		})
+		require.NoError(t, err)
+		CheckCreatedStatus(t, response)
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		require.NotNil(t, persisted)
+		require.True(t, persisted.SchemeAdmin)
+		return g
+	}
+
+	t.Run("regular channel user calling LINK with scheme_admin omitted must not change persisted scheme_admin", func(t *testing.T) {
+		g := seedSchemeAdminTrue(t)
+
+		patch := &model.GroupSyncablePatch{
+			AutoAdd: model.NewPointer(true),
+		}
+		_, _, _ = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		require.NotNil(t, persisted)
+		assert.True(t, persisted.SchemeAdmin)
+	})
+
+	t.Run("regular channel user calling LINK with scheme_admin: false must not change persisted scheme_admin", func(t *testing.T) {
+		g := seedSchemeAdminTrue(t)
+
+		patch := &model.GroupSyncablePatch{
+			AutoAdd:     model.NewPointer(true),
+			SchemeAdmin: model.NewPointer(false),
+		}
+		_, _, _ = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel)
+		require.NotNil(t, persisted)
+		assert.True(t, persisted.SchemeAdmin)
+	})
+}
+
+func TestLinkGroupTeam_LinkOnSoftDeletedDoesNotPreserveSchemeAdmin(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+
+	t.Run("regular team user re-linking a soft-deleted syncable with scheme_admin omitted must persist scheme_admin: false", func(t *testing.T) {
+		g := newSchemeAdminTestLdapGroup(t, th)
+
+		_, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{
+			AutoAdd:     model.NewPointer(true),
+			SchemeAdmin: model.NewPointer(true),
+		})
+		require.NoError(t, err)
+		CheckCreatedStatus(t, response)
+
+		response, err = th.SystemAdminClient.UnlinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NoError(t, err)
+		CheckOKStatus(t, response)
+
+		patch := &model.GroupSyncablePatch{
+			AutoAdd: model.NewPointer(true),
+		}
+		_, response, err = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch)
+		require.NoError(t, err)
+		CheckCreatedStatus(t, response)
+
+		persisted := findPersistedGroupSyncable(t, th, g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam)
+		require.NotNil(t, persisted)
+		assert.False(t, persisted.SchemeAdmin)
+	})
+}
+
+func TestPatchGroupTeam_OmittedSchemeAdminDoesNotDemoteDirectAdmin(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+
+	g := newSchemeAdminTestLdapGroup(t, th)
+
+	_, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{
+		AutoAdd: model.NewPointer(true),
+	})
+	require.NoError(t, err)
+	CheckCreatedStatus(t, response)
+
+	th.UpdateUserToTeamAdmin(t, th.BasicUser2, th.BasicTeam)
+
+	patch := &model.GroupSyncablePatch{
+		AutoAdd: model.NewPointer(false),
+	}
+	_, response, err = th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch)
+	require.NoError(t, err)
+	CheckOKStatus(t, response)
+
+	time.Sleep(2 * time.Second)
+
+	tm, appErr := th.App.GetTeamMember(th.Context, th.BasicTeam.Id, th.BasicUser2.Id)
+	require.Nil(t, appErr)
+	assert.True(t, tm.SchemeAdmin)
+}
+
+func TestPatchGroupChannel_OmittedSchemeAdminDoesNotDemoteDirectAdmin(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+
+	g := newSchemeAdminTestLdapGroup(t, th)
+
+	_, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{
+		AutoAdd: model.NewPointer(true),
+	})
+	require.NoError(t, err)
+	CheckCreatedStatus(t, response)
+
+	_, response, err = th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, &model.GroupSyncablePatch{
+		AutoAdd: model.NewPointer(true),
+	})
+	require.NoError(t, err)
+	CheckCreatedStatus(t, response)
+
+	th.MakeUserChannelAdmin(t, th.BasicUser2, th.BasicChannel)
+
+	patch := &model.GroupSyncablePatch{
+		AutoAdd: model.NewPointer(false),
+	}
+	_, response, err = th.Client.PatchGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch)
+	require.NoError(t, err)
+	CheckOKStatus(t, response)
+
+	time.Sleep(2 * time.Second)
+
+	cm, appErr := th.App.GetChannelMember(th.Context, th.BasicChannel.Id, th.BasicUser2.Id)
+	require.Nil(t, appErr)
+	assert.True(t, cm.SchemeAdmin)
+}
+
+func TestLinkGroupTeam_OmittedSchemeAdminDoesNotDemoteDirectAdmin(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+
+	g := newSchemeAdminTestLdapGroup(t, th)
+
+	th.UpdateUserToTeamAdmin(t, th.BasicUser2, th.BasicTeam)
+
+	patch := &model.GroupSyncablePatch{
+		AutoAdd: model.NewPointer(true),
+	}
+	_, response, err := th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, patch)
+	require.NoError(t, err)
+	CheckCreatedStatus(t, response)
+
+	time.Sleep(2 * time.Second)
+
+	tm, appErr := th.App.GetTeamMember(th.Context, th.BasicTeam.Id, th.BasicUser2.Id)
+	require.Nil(t, appErr)
+	assert.True(t, tm.SchemeAdmin)
+}
+
+func TestLinkGroupChannel_OmittedSchemeAdminDoesNotDemoteDirectAdmin(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+
+	g := newSchemeAdminTestLdapGroup(t, th)
+
+	_, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{
+		AutoAdd: model.NewPointer(true),
+	})
+	require.NoError(t, err)
+	CheckCreatedStatus(t, response)
+
+	th.MakeUserChannelAdmin(t, th.BasicUser2, th.BasicChannel)
+
+	patch := &model.GroupSyncablePatch{
+		AutoAdd: model.NewPointer(true),
+	}
+	_, response, err = th.Client.LinkGroupSyncable(context.Background(), g.Id, th.BasicChannel.Id, model.GroupSyncableTypeChannel, patch)
+	require.NoError(t, err)
+	CheckCreatedStatus(t, response)
+
+	time.Sleep(2 * time.Second)
+
+	cm, appErr := th.App.GetChannelMember(th.Context, th.BasicChannel.Id, th.BasicUser2.Id)
+	require.Nil(t, appErr)
+	assert.True(t, cm.SchemeAdmin)
+}
+
+func TestLinkGroupTeam_SchemeAdminTruePromotesGroupMembers(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+
+	g := newSchemeAdminTestLdapGroup(t, th)
+
+	_, appErr := th.App.UpsertGroupMember(g.Id, th.BasicUser2.Id)
+	require.Nil(t, appErr)
+
+	_, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{
+		AutoAdd:     model.NewPointer(true),
+		SchemeAdmin: model.NewPointer(true),
+	})
+	require.NoError(t, err)
+	CheckCreatedStatus(t, response)
+
+	time.Sleep(2 * time.Second)
+
+	tm, appErr := th.App.GetTeamMember(th.Context, th.BasicTeam.Id, th.BasicUser2.Id)
+	require.Nil(t, appErr)
+	assert.True(t, tm.SchemeAdmin)
+}
+
+func TestLinkGroupTeam_AutoAddOnlyAddsGroupMembers(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	th.App.Srv().SetLicense(model.NewTestLicense("ldap"))
+
+	g := newSchemeAdminTestLdapGroup(t, th)
+
+	newUser := th.CreateUser(t)
+	_, appErr := th.App.UpsertGroupMember(g.Id, newUser.Id)
+	require.Nil(t, appErr)
+
+	_, appErr = th.App.GetTeamMember(th.Context, th.BasicTeam.Id, newUser.Id)
+	require.NotNil(t, appErr)
+
+	_, response, err := th.SystemAdminClient.LinkGroupSyncable(context.Background(), g.Id, th.BasicTeam.Id, model.GroupSyncableTypeTeam, &model.GroupSyncablePatch{
+		AutoAdd: model.NewPointer(true),
+	})
+	require.NoError(t, err)
+	CheckCreatedStatus(t, response)
+
+	time.Sleep(2 * time.Second)
+
+	tm, appErr := th.App.GetTeamMember(th.Context, th.BasicTeam.Id, newUser.Id)
+	require.Nil(t, appErr)
+	assert.Equal(t, newUser.Id, tm.UserId)
+}

server/channels/app/syncables.go

@@ -268,18 +268,20 @@ func (a *App) SyncSyncableRoles(rctx request.CTX, syncableID string, syncableTyp
 	return nil
 }
 
-// SyncRolesAndMembership updates the SchemeAdmin status and membership of all of the members of the given
-// syncable.
-func (a *App) SyncRolesAndMembership(rctx request.CTX, syncableID string, syncableType model.GroupSyncableType, groupID string) {
+// SyncRolesAndMembership updates the membership of the given syncable and,
+// when syncRoles is true, also reconciles SchemeAdmin status for its members.
+func (a *App) SyncRolesAndMembership(rctx request.CTX, syncableID string, syncableType model.GroupSyncableType, groupID string, syncRoles bool) {
 	group, appErr := a.GetGroup(groupID, nil, nil)
 	if appErr != nil {
 		rctx.Logger().Warn("Error getting group", mlog.Err(appErr))
 		return
 	}
 
-	appErr = a.SyncSyncableRoles(rctx, syncableID, syncableType)
-	if appErr != nil {
-		rctx.Logger().Warn("Error syncing syncable roles", mlog.Err(appErr))
+	if syncRoles {
+		appErr = a.SyncSyncableRoles(rctx, syncableID, syncableType)
+		if appErr != nil {
+			rctx.Logger().Warn("Error syncing syncable roles", mlog.Err(appErr))
+		}
 	}
 
 	var since int64

server/channels/app/syncables_test.go

@@ -674,3 +674,147 @@ func TestSyncSyncableRoles(t *testing.T) {
 		require.True(t, cm.SchemeAdmin)
 	}
 }
+
+func TestSyncRolesAndMembership_RoleSyncGate(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	setup := func(t *testing.T) (*model.Team, *model.Channel, *model.Group, *model.User) {
+		t.Helper()
+
+		team := th.CreateTeam(t)
+		channel := th.CreateChannel(t, team)
+		group := th.CreateGroup(t)
+
+		_, err := th.App.UpsertGroupSyncable(&model.GroupSyncable{
+			SyncableId: team.Id,
+			Type:       model.GroupSyncableTypeTeam,
+			GroupId:    group.Id,
+			AutoAdd:    true,
+		})
+		require.Nil(t, err)
+
+		_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
+			SyncableId: channel.Id,
+			Type:       model.GroupSyncableTypeChannel,
+			GroupId:    group.Id,
+			AutoAdd:    true,
+		})
+		require.Nil(t, err)
+
+		directAdmin := th.CreateUser(t)
+		_, appErr := th.App.AddTeamMember(th.Context, team.Id, directAdmin.Id)
+		require.Nil(t, appErr)
+		_, appErr = th.App.AddUserToChannel(th.Context, directAdmin, channel, false)
+		require.Nil(t, appErr)
+
+		tm, storeErr := th.App.Srv().Store().Team().GetMember(th.Context, team.Id, directAdmin.Id)
+		require.NoError(t, storeErr)
+		tm.SchemeAdmin = true
+		_, storeErr = th.App.Srv().Store().Team().UpdateMember(th.Context, tm)
+		require.NoError(t, storeErr)
+
+		cm, storeErr := th.App.Srv().Store().Channel().GetMember(th.Context, channel.Id, directAdmin.Id)
+		require.NoError(t, storeErr)
+		cm.SchemeAdmin = true
+		_, storeErr = th.App.Srv().Store().Channel().UpdateMember(th.Context, cm)
+		require.NoError(t, storeErr)
+
+		return team, channel, group, directAdmin
+	}
+
+	t.Run("syncRoles=false preserves the existing SchemeAdmin on team members", func(t *testing.T) {
+		team, _, group, directAdmin := setup(t)
+
+		th.App.SyncRolesAndMembership(th.Context, team.Id, model.GroupSyncableTypeTeam, group.Id, false)
+
+		tm, appErr := th.App.GetTeamMember(th.Context, team.Id, directAdmin.Id)
+		require.Nil(t, appErr)
+		assert.True(t, tm.SchemeAdmin)
+	})
+
+	t.Run("syncRoles=false preserves the existing SchemeAdmin on channel members", func(t *testing.T) {
+		_, channel, group, directAdmin := setup(t)
+
+		th.App.SyncRolesAndMembership(th.Context, channel.Id, model.GroupSyncableTypeChannel, group.Id, false)
+
+		cm, appErr := th.App.GetChannelMember(th.Context, channel.Id, directAdmin.Id)
+		require.Nil(t, appErr)
+		assert.True(t, cm.SchemeAdmin)
+	})
+
+	t.Run("syncRoles=true reconciles team SchemeAdmin against PermittedSyncableAdmins", func(t *testing.T) {
+		team, _, group, directAdmin := setup(t)
+
+		th.App.SyncRolesAndMembership(th.Context, team.Id, model.GroupSyncableTypeTeam, group.Id, true)
+
+		tm, appErr := th.App.GetTeamMember(th.Context, team.Id, directAdmin.Id)
+		require.Nil(t, appErr)
+		assert.False(t, tm.SchemeAdmin)
+	})
+
+	t.Run("syncRoles=true reconciles channel SchemeAdmin against PermittedSyncableAdmins", func(t *testing.T) {
+		_, channel, group, directAdmin := setup(t)
+
+		th.App.SyncRolesAndMembership(th.Context, channel.Id, model.GroupSyncableTypeChannel, group.Id, true)
+
+		cm, appErr := th.App.GetChannelMember(th.Context, channel.Id, directAdmin.Id)
+		require.Nil(t, appErr)
+		assert.False(t, cm.SchemeAdmin)
+	})
+}
+
+func TestSyncRolesAndMembership_AlwaysSyncsMembership(t *testing.T) {
+	mainHelper.Parallel(t)
+	th := Setup(t).InitBasic(t)
+
+	setup := func(t *testing.T) (*model.Team, *model.Channel, *model.Group, *model.User) {
+		t.Helper()
+
+		team := th.CreateTeam(t)
+		channel := th.CreateChannel(t, team)
+		group := th.CreateGroup(t)
+
+		_, err := th.App.UpsertGroupSyncable(&model.GroupSyncable{
+			SyncableId: team.Id,
+			Type:       model.GroupSyncableTypeTeam,
+			GroupId:    group.Id,
+			AutoAdd:    true,
+		})
+		require.Nil(t, err)
+
+		_, err = th.App.UpsertGroupSyncable(&model.GroupSyncable{
+			SyncableId: channel.Id,
+			Type:       model.GroupSyncableTypeChannel,
+			GroupId:    group.Id,
+			AutoAdd:    true,
+		})
+		require.Nil(t, err)
+
+		groupMember := th.CreateUser(t)
+		_, err = th.App.UpsertGroupMember(group.Id, groupMember.Id)
+		require.Nil(t, err)
+
+		return team, channel, group, groupMember
+	}
+
+	t.Run("syncRoles=false still adds group members to the team", func(t *testing.T) {
+		team, _, group, groupMember := setup(t)
+
+		th.App.SyncRolesAndMembership(th.Context, team.Id, model.GroupSyncableTypeTeam, group.Id, false)
+
+		tm, appErr := th.App.GetTeamMember(th.Context, team.Id, groupMember.Id)
+		require.Nil(t, appErr)
+		assert.Equal(t, groupMember.Id, tm.UserId)
+	})
+
+	t.Run("syncRoles=false still adds group members to the channel", func(t *testing.T) {
+		_, channel, group, groupMember := setup(t)
+
+		th.App.SyncRolesAndMembership(th.Context, channel.Id, model.GroupSyncableTypeChannel, group.Id, false)
+
+		cm, appErr := th.App.GetChannelMember(th.Context, channel.Id, groupMember.Id)
+		require.Nil(t, appErr)
+		assert.Equal(t, groupMember.Id, cm.UserId)
+	})
+}

server/channels/store/sqlstore/group_store.go

@@ -798,6 +798,7 @@ func (s *SqlGroupStore) getGroupSyncable(groupID string, syncableID string, sync
 		groupSyncable.DeleteAt = groupTeam.DeleteAt
 		groupSyncable.UpdateAt = groupTeam.UpdateAt
 		groupSyncable.Type = syncableType
+		groupSyncable.SchemeAdmin = groupTeam.SchemeAdmin
 	case model.GroupSyncableTypeChannel:
 		groupChannel := result.(*groupChannel)
 		groupSyncable.SyncableId = groupChannel.ChannelId
@@ -807,6 +808,7 @@ func (s *SqlGroupStore) getGroupSyncable(groupID string, syncableID string, sync
 		groupSyncable.DeleteAt = groupChannel.DeleteAt
 		groupSyncable.UpdateAt = groupChannel.UpdateAt
 		groupSyncable.Type = syncableType
+		groupSyncable.SchemeAdmin = groupChannel.SchemeAdmin
 	default:
 		return nil, fmt.Errorf("unable to convert syncableType: %s", syncableType.String())
 	}
@@ -1836,7 +1838,7 @@ func (s *SqlGroupStore) AdminRoleGroupsForSyncableMember(userID, syncableID stri
 func (s *SqlGroupStore) PermittedSyncableAdmins(syncableID string, syncableType model.GroupSyncableType) ([]string, error) {
 	builder := s.getQueryBuilder().Select("UserId").
 		From(fmt.Sprintf("Group%ss", syncableType)).
-		Join(fmt.Sprintf("GroupMembers ON GroupMembers.GroupId = Group%ss.GroupId AND Group%[1]ss.SchemeAdmin = TRUE AND GroupMembers.DeleteAt = 0", syncableType.String())).Where(fmt.Sprintf("Group%[1]ss.%[1]sId = ?", syncableType.String()), syncableID)
+		Join(fmt.Sprintf("GroupMembers ON GroupMembers.GroupId = Group%ss.GroupId AND Group%[1]ss.SchemeAdmin = TRUE AND Group%[1]ss.DeleteAt = 0 AND GroupMembers.DeleteAt = 0", syncableType.String())).Where(fmt.Sprintf("Group%[1]ss.%[1]sId = ?", syncableType.String()), syncableID)
 
 	var userIDs []string
 	if err := s.GetMaster().SelectBuilder(&userIDs, builder); err != nil {

server/channels/store/storetest/group_store.go

@@ -1616,9 +1616,19 @@ func testGetGroupSyncable(t *testing.T, rctx request.CTX, ss store.Store) {
 	require.Equal(t, gt1.GroupId, dgt.GroupId)
 	require.Equal(t, gt1.SyncableId, dgt.SyncableId)
 	require.Equal(t, gt1.AutoAdd, dgt.AutoAdd)
+	require.Equal(t, gt1.SchemeAdmin, dgt.SchemeAdmin)
 	require.NotZero(t, gt1.CreateAt)
 	require.NotZero(t, gt1.UpdateAt)
 	require.Zero(t, gt1.DeleteAt)
+
+	// Round-trip SchemeAdmin: true through UpdateGroupSyncable and re-fetch.
+	dgt.SchemeAdmin = true
+	_, err = ss.Group().UpdateGroupSyncable(dgt)
+	require.NoError(t, err)
+
+	dgt, err = ss.Group().GetGroupSyncable(groupTeam.GroupId, groupTeam.SyncableId, model.GroupSyncableTypeTeam)
+	require.NoError(t, err)
+	require.True(t, dgt.SchemeAdmin, "GetGroupSyncable must populate SchemeAdmin from the persisted row")
 }
 
 func testGetGroupSyncableErrors(t *testing.T, rctx request.CTX, ss store.Store) {
@@ -4979,6 +4989,15 @@ func groupTestPermittedSyncableAdminsTeam(t *testing.T, rctx request.CTX, ss sto
 	// deleted group syncable no longer includes group members
 	_, err = ss.Group().DeleteGroupSyncable(group1.Id, team.Id, model.GroupSyncableTypeTeam)
 	require.NoError(t, err)
+
+	// The persisted row must still carry SchemeAdmin=true after soft-delete;
+	// PermittedSyncableAdmins excludes it via the DeleteAt = 0 predicate, not
+	// via the field having been silently cleared.
+	deletedSyncable, err := ss.Group().GetGroupSyncable(group1.Id, team.Id, model.GroupSyncableTypeTeam)
+	require.NoError(t, err)
+	require.True(t, deletedSyncable.SchemeAdmin)
+	require.NotZero(t, deletedSyncable.DeleteAt)
+
 	actualUserIDs, err = ss.Group().PermittedSyncableAdmins(team.Id, model.GroupSyncableTypeTeam)
 	require.NoError(t, err)
 	require.ElementsMatch(t, []string{user3.Id}, actualUserIDs)
@@ -5086,6 +5105,15 @@ func groupTestPermittedSyncableAdminsChannel(t *testing.T, rctx request.CTX, ss
 	// deleted group syncable no longer includes group members
 	_, err = ss.Group().DeleteGroupSyncable(group1.Id, channel.Id, model.GroupSyncableTypeChannel)
 	require.NoError(t, err)
+
+	// The persisted row must still carry SchemeAdmin=true after soft-delete;
+	// PermittedSyncableAdmins excludes it via the DeleteAt = 0 predicate, not
+	// via the field having been silently cleared.
+	deletedSyncable, err := ss.Group().GetGroupSyncable(group1.Id, channel.Id, model.GroupSyncableTypeChannel)
+	require.NoError(t, err)
+	require.True(t, deletedSyncable.SchemeAdmin)
+	require.NotZero(t, deletedSyncable.DeleteAt)
+
 	actualUserIDs, err = ss.Group().PermittedSyncableAdmins(channel.Id, model.GroupSyncableTypeChannel)
 	require.NoError(t, err)
 	require.ElementsMatch(t, []string{user3.Id}, actualUserIDs)

webapp/channels/src/components/admin_console/group_settings/group_details/group_details.tsx

@@ -417,17 +417,25 @@ class GroupDetails extends React.PureComponent<Props, State> {
 
     roleChangeKey = (groupTeamOrChannel: {
         type?: SyncableType;
+        id?: string;
         team_id?: string;
         channel_id?: string;
     }) => {
-        let id;
-        if (
-            this.syncableTypeFromEntryType(groupTeamOrChannel.type) ===
-            SyncableType.Team
-        ) {
-            id = groupTeamOrChannel.team_id;
-        } else {
-            id = groupTeamOrChannel.channel_id;
+        // Items in itemsToRemove use a generic `id`, while items coming from
+        // teamsToAdd/channelsToAdd use `team_id`/`channel_id`. The key must
+        // be identical regardless of source so the dedup in
+        // handleRemovedTeamsAndChannels and handleAddedTeamsAndChannels
+        // matches the key produced by onChangeRoles.
+        let id = groupTeamOrChannel.id;
+        if (!id) {
+            if (
+                this.syncableTypeFromEntryType(groupTeamOrChannel.type) ===
+                SyncableType.Team
+            ) {
+                id = groupTeamOrChannel.team_id;
+            } else {
+                id = groupTeamOrChannel.channel_id;
+            }
         }
         return `${id}/${groupTeamOrChannel.type}`;
     };