Don't remove system admins from channels even if they "shouldn't" be in them

Author: lukegb

server/syncablesync/mattermostservice.go

@@ -420,6 +420,24 @@ func (m *Mattermost) FetchGroupsAndSyncables(ctx context.Context) ([]Group, erro
 	return groups, nil
 }
 
+func (m *Mattermost) UnremovableUserIDs(ctx context.Context) ([]string, error) {
+	// For Mattermost, just use 'is system admin' as a proxy for 'should not be automatically removed from channels they shouldn't be in'.
+	// Note that if someone is promoted to a system admin at the same time as they would be removed from channels, then they will be removed in _that_ sync run.
+	srh := &mattermostSystemRoleHandler{api: m.API}
+	rms, err := srh.FetchRoster(ctx, SyncableTarget{
+		Type: mattermostSyncableTypeSystemRole,
+		ID:   "system_admin",
+	})
+	if err != nil {
+		return nil, err
+	}
+	out := make([]string, len(rms))
+	for n, rm := range rms {
+		out[n] = rm.UserID
+	}
+	return out, nil
+}
+
 func (m *Mattermost) getSystemBot(ctx context.Context) (*model.User, error) {
 	l := ctxlog.FromContext(ctx)
 	u, appErr := m.API.GetUserByUsername(model.BotSystemBotUsername)

server/syncablesync/syncablesync.go

@@ -13,12 +13,14 @@ import (
 	"slices"
 
 	"github.com/lukegb/mattermost-plugin-uffd/server/ctxlog"
+	"github.com/lukegb/mattermost-plugin-uffd/server/stringset"
 )
 
 type ServiceAPI interface {
 	SortSyncableTargets([]SyncableTarget)
 	SyncableHandlers() map[SyncableType]SyncableHandler
 	FetchGroupsAndSyncables(context.Context) ([]Group, error)
+	UnremovableUserIDs(context.Context) ([]string, error)
 }
 
 type Engine struct {
@@ -89,7 +91,7 @@ func (e *Engine) FullSync(ctx context.Context) error {
 
 	groups, err := e.API.FetchGroupsAndSyncables(ctx)
 	if err != nil {
-		return fmt.Errorf("fetching list of groups to sync: %v", err)
+		return fmt.Errorf("fetching list of groups to sync: %w", err)
 	}
 	l.WithField("groups", groups).Debug("found groups")
 
@@ -102,6 +104,13 @@ func (e *Engine) FullSync(ctx context.Context) error {
 		}
 	}
 
+	// Users which should not be removed from channels that they 'shouldn't be in'.
+	unremovableUsersList, err := e.API.UnremovableUserIDs(ctx)
+	if err != nil {
+		return fmt.Errorf("fetching list of 'unremovable' user IDs: %w", err)
+	}
+	unremovableUsers := stringset.FromSlice(unremovableUsersList)
+
 	sortedSyncableTarget := slices.Collect(maps.Keys(syncablesToGroups))
 	e.API.SortSyncableTargets(sortedSyncableTarget)
 
@@ -152,7 +161,12 @@ func (e *Engine) FullSync(ctx context.Context) error {
 		if !addOnly {
 			for uid, gotRM := range gotRosterMap {
 				if _, ok := wantRosterMap[uid]; !ok {
-					membersToDelete = append(membersToDelete, gotRM)
+					if unremovableUsers.Contains(uid) {
+						gotRM.IsAdmin = false
+						membersToUpdate = append(membersToUpdate, gotRM)
+					} else {
+						membersToDelete = append(membersToDelete, gotRM)
+					}
 				}
 			}
 		} else {

server/syncablesync/syncablesync_test.go

@@ -15,6 +15,7 @@ const dummySyncableType = SyncableType("flarp")
 type dummyService struct {
 	groups          []Group
 	syncableMembers map[SyncableTarget][]RosterMember
+	unremovable     []string
 }
 
 var _ ServiceAPI = ((*dummyService)(nil))
@@ -31,6 +32,10 @@ func (s *dummyService) FetchGroupsAndSyncables(context.Context) ([]Group, error)
 	return s.groups, nil
 }
 
+func (s *dummyService) UnremovableUserIDs(context.Context) ([]string, error) {
+	return s.unremovable, nil
+}
+
 type dummyServiceHandler struct {
 	s *dummyService
 }
@@ -219,6 +224,34 @@ func TestFullSync(t *testing.T) {
 				UserID: "user2",
 			}},
 		},
+	}, {
+		name: "doesn't remove 'unremovable' members (but does demote them)",
+		s: &dummyService{
+			unremovable: []string{"user1", "user2"},
+			groups: []Group{{
+				ID:   "group",
+				Name: "group name",
+				// no Members
+				Syncables: []Syncable{{
+					Target: dummyST,
+				}},
+			}},
+			syncableMembers: map[SyncableTarget][]RosterMember{
+				dummyST: {{
+					UserID:  "user1",
+					IsAdmin: true,
+				}, {
+					UserID: "user2",
+				}},
+			},
+		},
+		wantSyncableMembers: map[SyncableTarget][]RosterMember{
+			dummyST: {{
+				UserID: "user1",
+			}, {
+				UserID: "user2",
+			}},
+		},
 	}}
 	for _, tc := range tcs {
 		t.Run(tc.name, func(t *testing.T) {