Adds logical deletes to shared channel remotes and remote clusters (#28159)

* Adds logical deletes to shared channel remotes and remote clusters

Instead of physically deleting the shared channel remote and remote
clusters records when a channel is unshared, a remote uninvited or a
remote cluster is deleted, now those have a logical `DeleteAt` field
that is set.

This allows us to safely restore shared channels between two remote
clusters (as of now resetting the cursor without backfilling their
contents) and to know which connections were established in the past
and now are severed.

* Delete the index in remoteclusters before adding the new column

* Fix bad error check

server/channels/api4/remote_cluster.go

@@ -325,6 +325,7 @@ func getRemoteClusters(c *Context, w http.ResponseWriter, r *http.Request) {
 		PluginID:       c.Params.PluginId,
 		OnlyPlugins:    c.Params.OnlyPlugins,
 		ExcludePlugins: c.Params.ExcludePlugins,
+		IncludeDeleted: c.Params.IncludeDeleted,
 	}
 
 	rcs, appErr := c.App.GetAllRemoteClusters(c.Params.Page, c.Params.PerPage, filter)

server/channels/api4/remote_cluster_test.go

@@ -5,7 +5,6 @@ package api4
 
 import (
 	"context"
-	"database/sql"
 	"encoding/base64"
 	"testing"
 
@@ -48,6 +47,13 @@ func TestGetRemoteClusters(t *testing.T) {
 			CreatorId: th.SystemAdminUser.Id,
 			PluginID:  model.NewId(),
 		},
+		{
+			RemoteId:  model.NewId(),
+			Name:      "remote4",
+			SiteURL:   "http://example4.com",
+			CreatorId: th.SystemAdminUser.Id,
+			DeleteAt:  123,
+		},
 	}
 
 	for _, rc := range newRCs {
@@ -94,6 +100,16 @@ func TestGetRemoteClusters(t *testing.T) {
 			ExpectedError:      false,
 			ExpectedNames:      []string{"remote1", "remote2", "remote3"},
 		},
+		{
+			Name:               "Should return all remote clusters including deleted",
+			Client:             th.SystemAdminClient,
+			Page:               0,
+			PerPage:            999999,
+			Filter:             model.RemoteClusterQueryFilter{IncludeDeleted: true},
+			ExpectedStatusCode: 200,
+			ExpectedError:      false,
+			ExpectedNames:      []string{"remote1", "remote2", "remote3", "remote4"},
+		},
 		{
 			Name:               "Should return all remote clusters but those belonging to plugins",
 			Client:             th.SystemAdminClient,
@@ -104,6 +120,16 @@ func TestGetRemoteClusters(t *testing.T) {
 			ExpectedError:      false,
 			ExpectedNames:      []string{"remote1", "remote2"},
 		},
+		{
+			Name:               "Should return all remote clusters but those belonging to plugins, including deleted",
+			Client:             th.SystemAdminClient,
+			Page:               0,
+			PerPage:            999999,
+			Filter:             model.RemoteClusterQueryFilter{ExcludePlugins: true, IncludeDeleted: true},
+			ExpectedStatusCode: 200,
+			ExpectedError:      false,
+			ExpectedNames:      []string{"remote1", "remote2", "remote4"},
+		},
 		{
 			Name:               "Should return only remote clusters belonging to plugins",
 			Client:             th.SystemAdminClient,
@@ -572,18 +598,19 @@ func TestDeleteRemoteCluster(t *testing.T) {
 	})
 
 	t.Run("should correctly delete the remote cluster", func(t *testing.T) {
+		// ensure the remote cluster is not deleted
+		initialRC, appErr := th.App.GetRemoteCluster(rc.RemoteId)
+		require.Nil(t, appErr)
+		require.NotEmpty(t, initialRC)
+		require.Zero(t, initialRC.DeleteAt)
+
 		resp, err := th.SystemAdminClient.DeleteRemoteCluster(context.Background(), rc.RemoteId)
 		CheckNoContentStatus(t, resp)
 		require.NoError(t, err)
 
-		deletedRC, err := th.App.GetRemoteCluster(rc.RemoteId)
-		require.ErrorIs(t, err, sql.ErrNoRows)
-		require.Empty(t, deletedRC)
-	})
-
-	t.Run("should return not found if the remote cluster is already deleted", func(t *testing.T) {
-		resp, err := th.SystemAdminClient.DeleteRemoteCluster(context.Background(), rc.RemoteId)
-		CheckNotFoundStatus(t, resp)
-		require.Error(t, err)
+		deletedRC, appErr := th.App.GetRemoteCluster(rc.RemoteId)
+		require.Nil(t, appErr)
+		require.NotEmpty(t, deletedRC)
+		require.NotZero(t, deletedRC.DeleteAt)
 	})
 }

server/channels/api4/shared_channel.go

@@ -116,9 +116,10 @@ func getSharedChannelRemotesByRemoteCluster(c *Context, w http.ResponseWriter, r
 	}
 
 	filter := model.SharedChannelRemoteFilterOpts{
-		RemoteId:      c.Params.RemoteId,
-		ExcludeHome:   c.Params.ExcludeHome,
-		ExcludeRemote: c.Params.ExcludeRemote,
+		RemoteId:       c.Params.RemoteId,
+		ExcludeHome:    c.Params.ExcludeHome,
+		ExcludeRemote:  c.Params.ExcludeRemote,
+		IncludeDeleted: c.Params.IncludeDeleted,
 	}
 	sharedChannelRemotes, err := c.App.GetSharedChannelRemotes(c.Params.Page, c.Params.PerPage, filter)
 	if err != nil {
@@ -153,7 +154,7 @@ func inviteRemoteClusterToChannel(c *Context, w http.ResponseWriter, r *http.Req
 		return
 	}
 
-	if _, appErr := c.App.GetRemoteCluster(c.Params.RemoteId); appErr != nil {
+	if rc, appErr := c.App.GetRemoteCluster(c.Params.RemoteId); appErr != nil || rc.DeleteAt != 0 {
 		c.SetInvalidRemoteIdError(c.Params.RemoteId)
 		return
 	}
@@ -200,7 +201,7 @@ func uninviteRemoteClusterToChannel(c *Context, w http.ResponseWriter, r *http.R
 		return
 	}
 
-	if _, appErr := c.App.GetRemoteCluster(c.Params.RemoteId); appErr != nil {
+	if rc, appErr := c.App.GetRemoteCluster(c.Params.RemoteId); appErr != nil || rc.DeleteAt != 0 {
 		c.SetInvalidRemoteIdError(c.Params.RemoteId)
 		return
 	}

server/channels/api4/shared_channel_test.go

@@ -299,6 +299,20 @@ func TestGetSharedChannelRemotesByRemoteCluster(t *testing.T) {
 	_, err = th.App.ShareChannel(th.Context, sc3)
 	require.NoError(t, err)
 
+	c4 := th.CreateChannelWithClientAndTeam(th.Client, model.ChannelTypeOpen, th.BasicTeam.Id)
+	sc4 := &model.SharedChannel{
+		ChannelId:        c4.Id,
+		TeamId:           th.BasicTeam.Id,
+		ShareName:        "shared_4",
+		ShareDisplayName: "Shared Channel 4",
+		CreatorId:        th.BasicUser.Id,
+		RemoteId:         rc1.RemoteId,
+		Home:             false,
+	}
+
+	_, err = th.App.ShareChannel(th.Context, sc4)
+	require.NoError(t, err)
+
 	// for the pagination test, we need to get the channelId of the
 	// second SharedChannelRemote that belongs to RC1, sorted by ID,
 	// so we accumulate those SharedChannelRemotes on creation and
@@ -307,7 +321,7 @@ func TestGetSharedChannelRemotesByRemoteCluster(t *testing.T) {
 	sharedChannelRemotesFromRC1 := []*model.SharedChannelRemote{}
 
 	// create the shared channel remotes
-	for _, sc := range []*model.SharedChannel{sc1, sc2, sc3} {
+	for _, sc := range []*model.SharedChannel{sc1, sc2, sc3, sc4} {
 		scr := &model.SharedChannelRemote{
 			Id:                model.NewId(),
 			ChannelId:         sc.ChannelId,
@@ -324,6 +338,14 @@ func TestGetSharedChannelRemotesByRemoteCluster(t *testing.T) {
 		}
 	}
 
+	// we delete the shared channel remote for sc4
+	scr4, err := th.App.GetSharedChannelRemoteByIds(sc4.ChannelId, sc4.RemoteId)
+	require.NoError(t, err)
+
+	deleted, err := th.App.DeleteSharedChannelRemote(scr4.Id)
+	require.NoError(t, err)
+	require.True(t, deleted)
+
 	sort.Slice(sharedChannelRemotesFromRC1, func(i, j int) bool {
 		return sharedChannelRemotesFromRC1[i].Id < sharedChannelRemotesFromRC1[j].Id
 	})
@@ -335,6 +357,7 @@ func TestGetSharedChannelRemotesByRemoteCluster(t *testing.T) {
 			RemoteId           string
 			ExcludeHome        bool
 			ExcludeRemote      bool
+			IncludeDeleted     bool
 			Page               int
 			PerPage            int
 			ExpectedStatusCode int
@@ -369,6 +392,17 @@ func TestGetSharedChannelRemotesByRemoteCluster(t *testing.T) {
 				ExpectedError:      false,
 				ExpectedIds:        []string{sc1.ChannelId, sc2.ChannelId},
 			},
+			{
+				Name:               "should return the complete list of shared channel remotes for a remote cluster, including deleted",
+				Client:             th.SystemAdminClient,
+				RemoteId:           rc1.RemoteId,
+				IncludeDeleted:     true,
+				Page:               0,
+				PerPage:            100,
+				ExpectedStatusCode: http.StatusOK,
+				ExpectedError:      false,
+				ExpectedIds:        []string{sc1.ChannelId, sc2.ChannelId, sc4.ChannelId},
+			},
 			{
 				Name:               "should return only the shared channel remotes homed localy",
 				Client:             th.SystemAdminClient,
@@ -395,6 +429,7 @@ func TestGetSharedChannelRemotesByRemoteCluster(t *testing.T) {
 				Name:               "should correctly paginate the results",
 				Client:             th.SystemAdminClient,
 				RemoteId:           rc1.RemoteId,
+				IncludeDeleted:     true,
 				Page:               1,
 				PerPage:            1,
 				ExpectedStatusCode: http.StatusOK,
@@ -405,7 +440,7 @@ func TestGetSharedChannelRemotesByRemoteCluster(t *testing.T) {
 
 		for _, tc := range testCases {
 			t.Run(tc.Name, func(t *testing.T) {
-				scrs, resp, err := tc.Client.GetSharedChannelRemotesByRemoteCluster(context.Background(), tc.RemoteId, tc.ExcludeHome, tc.ExcludeRemote, tc.Page, tc.PerPage)
+				scrs, resp, err := tc.Client.GetSharedChannelRemotesByRemoteCluster(context.Background(), tc.RemoteId, tc.ExcludeHome, tc.ExcludeRemote, tc.IncludeDeleted, tc.Page, tc.PerPage)
 				checkHTTPStatus(t, resp, tc.ExpectedStatusCode)
 				if tc.ExpectedError {
 					require.Error(t, err)

server/channels/app/remote_cluster.go

@@ -30,10 +30,20 @@ func (a *App) RegisterPluginForSharedChannels(rctx request.CTX, opts model.Regis
 
 	// if plugin is already registered then treat this as an update.
 	if rc != nil {
-		rctx.Logger().Debug("Plugin already registered for Shared Channels",
-			mlog.String("plugin_id", opts.PluginID),
-			mlog.String("remote_id", rc.RemoteId),
-		)
+		// plugin was deleted at some point
+		if rc.DeleteAt != 0 {
+			rctx.Logger().Debug("Restoring plugin registration for Shared Channels",
+				mlog.String("plugin_id", opts.PluginID),
+				mlog.String("remote_id", rc.RemoteId),
+			)
+
+			rc.DeleteAt = 0
+		} else {
+			rctx.Logger().Debug("Plugin already registered for Shared Channels",
+				mlog.String("plugin_id", opts.PluginID),
+				mlog.String("remote_id", rc.RemoteId),
+			)
+		}
 
 		rc.DisplayName = opts.Displayname
 		rc.Options = opts.GetOptionFlags()
@@ -82,6 +92,11 @@ func (a *App) UnregisterPluginForSharedChannels(pluginID string) error {
 		return err
 	}
 
+	if rc.DeleteAt != 0 {
+		// plugin already unregistered, nothing to do
+		return nil
+	}
+
 	_, appErr := a.DeleteRemoteCluster(rc.RemoteId)
 	if appErr != nil {
 		return appErr

server/channels/app/remote_cluster_test.go

@@ -6,11 +6,9 @@ package app
 import (
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/mattermost/mattermost/server/public/model"
-	"github.com/mattermost/mattermost/server/public/shared/i18n"
 )
 
 func setupRemoteCluster(tb testing.TB) *TestHelper {
@@ -38,8 +36,7 @@ func TestAddRemoteCluster(t *testing.T) {
 
 		remoteCluster.RemoteId = model.NewId()
 		_, err = th.App.AddRemoteCluster(remoteCluster)
-		require.NotNil(t, err, "Adding a duplicate remote cluster should error")
-		assert.Contains(t, err.Error(), i18n.T("api.remote_cluster.save_not_unique.app_error"))
+		require.Nil(t, err, "Adding a duplicate remote cluster should work fine")
 	})
 }
 
@@ -74,8 +71,7 @@ func TestUpdateRemoteCluster(t *testing.T) {
 
 		savedRemoteClustered.SiteURL = remoteCluster.SiteURL
 		_, err = th.App.UpdateRemoteCluster(savedRemoteClustered)
-		require.NotNil(t, err, "Updating remote cluster with duplicate site url should error")
-		assert.Contains(t, err.Error(), i18n.T("api.remote_cluster.update_not_unique.app_error"))
+		require.Nil(t, err, "Updating remote cluster with duplicate site url should work fine")
 	})
 
 	t.Run("update remote cluster with an already existing site url, is not allowed", func(t *testing.T) {
@@ -106,7 +102,6 @@ func TestUpdateRemoteCluster(t *testing.T) {
 		// Same site url
 		anotherExistingRemoteClustered.SiteURL = existingRemoteCluster.SiteURL
 		_, err = th.App.UpdateRemoteCluster(anotherExistingRemoteClustered)
-		require.NotNil(t, err, "Updating remote cluster should error")
-		assert.Contains(t, err.Error(), i18n.T("api.remote_cluster.update_not_unique.app_error"))
+		require.Nil(t, err, "Updating remote cluster should work fine")
 	})
 }

server/channels/app/session.go

@@ -69,7 +69,7 @@ func (a *App) GetCloudSession(token string) (*model.Session, *model.AppError) {
 
 func (a *App) GetRemoteClusterSession(token string, remoteId string) (*model.Session, *model.AppError) {
 	rc, appErr := a.GetRemoteCluster(remoteId)
-	if appErr == nil && subtle.ConstantTimeCompare([]byte(rc.Token), []byte(token)) == 1 {
+	if appErr == nil && rc.DeleteAt == 0 && subtle.ConstantTimeCompare([]byte(rc.Token), []byte(token)) == 1 {
 		// Need a bare-bones session object for later checks
 		session := &model.Session{
 			Token:   token,

server/channels/app/slashcommands/command_remote.go

@@ -219,7 +219,7 @@ func (rp *RemoteProvider) doRemove(a *app.App, args *model.CommandArgs, margs ma
 
 // doStatus displays connection status for all remote clusters.
 func (rp *RemoteProvider) doStatus(a *app.App, args *model.CommandArgs, _ map[string]string) *model.CommandResponse {
-	list, err := a.GetAllRemoteClusters(0, 999999, model.RemoteClusterQueryFilter{})
+	list, err := a.GetAllRemoteClusters(0, 999999, model.RemoteClusterQueryFilter{IncludeDeleted: true})
 	if err != nil {
 		responsef(args.T("api.command_remote.fetch_status.error", map[string]any{"Error": err.Error()}))
 	}
@@ -230,15 +230,16 @@ func (rp *RemoteProvider) doStatus(a *app.App, args *model.CommandArgs, _ map[st
 
 	var sb strings.Builder
 	fmt.Fprintf(&sb, args.T("api.command_remote.remote_table_header")+" \n")
-	// | Secure Connection | Display name | ConnectionID | Site URL | Invite accepted | Online | Last ping  |
-	fmt.Fprintf(&sb, "| :---- | :---- | :---- | :---- | :---- | :---- | :---- | \n")
+	// | Secure Connection | Display name | ConnectionID | Site URL | Default Team | Invite accepted | Online | Last ping | Deleted |
+	fmt.Fprintf(&sb, "| :---- | :---- | :---- | :---- | :---- | :---- | :---- | :---- | | :---- |\n")
 
 	for _, rc := range list {
 		accepted := formatBool(args.T, rc.IsConfirmed())
 		online := formatBool(args.T, isOnline(rc.LastPingAt))
 		lastPing := formatTimestamp(rc.LastPingAt)
+		deleted := formatBool(args.T, rc.DeleteAt != 0)
 
-		fmt.Fprintf(&sb, "| %s | %s | %s | %s | %s | %s | %s |\n", rc.Name, rc.DisplayName, rc.RemoteId, rc.GetSiteURL(), accepted, online, lastPing)
+		fmt.Fprintf(&sb, "| %s | %s | %s | %s | %s | %s | %s | %s | %s |\n", rc.Name, rc.DisplayName, rc.RemoteId, rc.GetSiteURL(), rc.DefaultTeamId, accepted, online, lastPing, deleted)
 	}
 	return responsef(sb.String())
 }

server/channels/app/slashcommands/command_share.go

@@ -252,6 +252,9 @@ func (sp *ShareProvider) doInviteRemote(a *app.App, c request.CTX, args *model.C
 	if appErr != nil {
 		return responsef(args.T("api.command_share.remote_id_invalid.error", map[string]any{"Error": appErr.Error()}))
 	}
+	if rc.DeleteAt != 0 {
+		return responsef(args.T("api.command_share.remote_id_invalid.error", map[string]any{"Error": "entity is deleted"}))
+	}
 
 	if err = a.InviteRemoteToChannel(args.ChannelId, remoteID, args.UserId, true); err != nil {
 		return responsef(args.T("api.command_share.invite_remote_to_channel.error", map[string]any{"Error": err.Error()}))

server/channels/db/migrations/migrations.list

@@ -247,6 +247,8 @@ channels/db/migrations/mysql/000124_remove_manage_team_permission.down.sql
 channels/db/migrations/mysql/000124_remove_manage_team_permission.up.sql
 channels/db/migrations/mysql/000125_remoteclusters_add_default_team_id.down.sql
 channels/db/migrations/mysql/000125_remoteclusters_add_default_team_id.up.sql
+channels/db/migrations/mysql/000126_sharedchannels_remotes_add_deleteat.down.sql
+channels/db/migrations/mysql/000126_sharedchannels_remotes_add_deleteat.up.sql
 channels/db/migrations/postgres/000001_create_teams.down.sql
 channels/db/migrations/postgres/000001_create_teams.up.sql
 channels/db/migrations/postgres/000002_create_team_members.down.sql
@@ -495,3 +497,5 @@ channels/db/migrations/postgres/000124_remove_manage_team_permission.down.sql
 channels/db/migrations/postgres/000124_remove_manage_team_permission.up.sql
 channels/db/migrations/postgres/000125_remoteclusters_add_default_team_id.down.sql
 channels/db/migrations/postgres/000125_remoteclusters_add_default_team_id.up.sql
+channels/db/migrations/postgres/000126_sharedchannels_remotes_add_deleteat.down.sql
+channels/db/migrations/postgres/000126_sharedchannels_remotes_add_deleteat.up.sql

server/channels/db/migrations/mysql/000126_sharedchannels_remotes_add_deleteat.down.sql

@@ -0,0 +1 @@
+-- Skipping it because the forward migrations are destructive

server/channels/db/migrations/mysql/000126_sharedchannels_remotes_add_deleteat.up.sql

@@ -0,0 +1,44 @@
+SET @preparedStatement = (SELECT IF(
+    (
+        SELECT COUNT(*) FROM INFORMATION_SCHEMA.STATISTICS
+        WHERE table_name = 'RemoteClusters'
+        AND table_schema = DATABASE()
+        AND index_name = 'remote_clusters_site_url_unique'
+    ) > 0,
+    'DROP INDEX remote_clusters_site_url_unique ON RemoteClusters;',
+    'SELECT 1'
+));
+
+PREPARE removeIndexIfExists FROM @preparedStatement;
+EXECUTE removeIndexIfExists;
+DEALLOCATE PREPARE removeIndexIfExists;
+
+SET @preparedStatement = (SELECT IF(
+    (
+        SELECT COUNT(*) FROM INFORMATION_SCHEMA.COLUMNS
+        WHERE table_name = 'RemoteClusters'
+        AND table_schema = DATABASE()
+        AND column_name = 'DeleteAt'
+    ) > 0,
+    'SELECT 1',
+    'ALTER TABLE RemoteClusters ADD DeleteAt bigint(20) DEFAULT 0;'
+));
+
+PREPARE alterIfNotExists FROM @preparedStatement;
+EXECUTE alterIfNotExists;
+DEALLOCATE PREPARE alterIfNotExists;
+
+SET @preparedStatement = (SELECT IF(
+    (
+        SELECT COUNT(*) FROM INFORMATION_SCHEMA.COLUMNS
+        WHERE table_name = 'SharedChannelRemotes'
+        AND table_schema = DATABASE()
+        AND column_name = 'DeleteAt'
+    ) > 0,
+    'SELECT 1',
+    'ALTER TABLE SharedChannelRemotes ADD DeleteAt bigint(20) DEFAULT 0;'
+));
+
+PREPARE alterIfNotExists FROM @preparedStatement;
+EXECUTE alterIfNotExists;
+DEALLOCATE PREPARE alterIfNotExists;

server/channels/db/migrations/postgres/000126_sharedchannels_remotes_add_deleteat.down.sql

@@ -0,0 +1 @@
+-- Skipping it because the forward migrations are destructive

server/channels/db/migrations/postgres/000126_sharedchannels_remotes_add_deleteat.up.sql

@@ -0,0 +1,4 @@
+DROP INDEX IF EXISTS remote_clusters_site_url_unique;
+ALTER TABLE remoteclusters ADD COLUMN IF NOT EXISTS deleteat bigint DEFAULT 0;
+
+ALTER TABLE sharedchannelremotes ADD COLUMN IF NOT EXISTS deleteat bigint DEFAULT 0;

server/channels/store/sqlstore/remote_cluster_store.go

@@ -36,6 +36,7 @@ func remoteClusterFields(prefix string) []string {
 		prefix + "SiteURL",
 		prefix + "DefaultTeamId",
 		prefix + "CreateAt",
+		prefix + "DeleteAt",
 		prefix + "LastPingAt",
 		prefix + "Token",
 		prefix + "RemoteToken",
@@ -67,10 +68,10 @@ func (s sqlRemoteClusterStore) Save(remoteCluster *model.RemoteCluster) (*model.
 
 	query := `INSERT INTO RemoteClusters
 				(RemoteId, RemoteTeamId, Name, DisplayName, SiteURL, DefaultTeamId, CreateAt,
-				LastPingAt, Token, RemoteToken, Topics, CreatorId, PluginID, Options)
+                DeleteAt, LastPingAt, Token, RemoteToken, Topics, CreatorId, PluginID, Options)
 				VALUES
 				(:RemoteId, :RemoteTeamId, :Name, :DisplayName, :SiteURL, :DefaultTeamId, :CreateAt,
-				:LastPingAt, :Token, :RemoteToken, :Topics, :CreatorId, :PluginID, :Options)`
+				:DeleteAt, :LastPingAt, :Token, :RemoteToken, :Topics, :CreatorId, :PluginID, :Options)`
 
 	if _, err := s.GetMasterX().NamedExec(query, remoteCluster); err != nil {
 		return nil, errors.Wrap(err, "failed to save RemoteCluster")
@@ -89,6 +90,7 @@ func (s sqlRemoteClusterStore) Update(remoteCluster *model.RemoteCluster) (*mode
 			SET Token = :Token,
 			RemoteTeamId = :RemoteTeamId,
 			CreateAt = :CreateAt,
+            DeleteAt = :DeleteAt,
 			LastPingAt = :LastPingAt,
 			RemoteToken = :RemoteToken,
 			CreatorId = :CreatorId,
@@ -107,24 +109,53 @@ func (s sqlRemoteClusterStore) Update(remoteCluster *model.RemoteCluster) (*mode
 }
 
 func (s sqlRemoteClusterStore) Delete(remoteId string) (bool, error) {
+	transaction, err := s.GetMasterX().Beginx()
+	if err != nil {
+		return false, errors.Wrap(err, "DeleteRemoteCluster: begin_transaction")
+	}
+	defer finalizeTransactionX(transaction, &err)
+
+	curTime := model.GetMillis()
+
+	// we delete the remote cluster itself
 	squery, args, err := s.getQueryBuilder().
-		Delete("RemoteClusters").
+		Update("RemoteClusters").
+		Set("DeleteAt", curTime).
 		Where(sq.Eq{"RemoteId": remoteId}).
 		ToSql()
 	if err != nil {
 		return false, errors.Wrap(err, "delete_remote_cluster_tosql")
 	}
 
-	result, err := s.GetMasterX().Exec(squery, args...)
+	result, err := transaction.Exec(squery, args...)
 	if err != nil {
 		return false, errors.Wrap(err, "failed to delete RemoteCluster")
 	}
 
+	// also remove the shared channel remotes for the cluster (if any)
+	squery, args, err = s.getQueryBuilder().
+		Update("SharedChannelRemotes").
+		Set("UpdateAt", curTime).
+		Set("DeleteAt", curTime).
+		Where(sq.Eq{"RemoteId": remoteId}).
+		ToSql()
+	if err != nil {
+		return false, errors.Wrap(err, "delete_shared_channel_remotes_for_remote_cluster_tosql")
+	}
+
+	if _, err = transaction.Exec(squery, args...); err != nil {
+		return false, errors.Wrap(err, "failed to delete SharedChannelRemotes for RemoteCluster")
+	}
+
 	count, err := result.RowsAffected()
 	if err != nil {
 		return false, errors.Wrap(err, "failed to determine rows affected")
 	}
 
+	if err = transaction.Commit(); err != nil {
+		return false, errors.Wrap(err, "commit_transaction")
+	}
+
 	return count > 0, nil
 }
 
@@ -213,6 +244,10 @@ func (s sqlRemoteClusterStore) GetAll(offset, limit int, filter model.RemoteClus
 		query = query.Where(sq.NotEq{fmt.Sprintf("(rc.Options & %d)", filter.RequireOptions): 0})
 	}
 
+	if !filter.IncludeDeleted {
+		query = query.Where(sq.Eq{"DeleteAt": 0})
+	}
+
 	if filter.Topic != "" {
 		trimmed := strings.TrimSpace(filter.Topic)
 		if trimmed == "" || trimmed == "*" {

server/channels/store/sqlstore/shared_channel_store.go

@@ -269,8 +269,10 @@ func (s SqlSharedChannelStore) Update(sc *model.SharedChannel) (*model.SharedCha
 	return sc, nil
 }
 
-// Delete deletes a single shared channel plus associated SharedChannelRemotes.
-// Returns true if shared channel found and deleted, false if not found.
+// Delete deletes a single shared channel as deleted, plus marks as
+// deleted associated SharedChannelRemotes.
+// Returns true if shared channel found and deleted, false if not
+// found.
 func (s SqlSharedChannelStore) Delete(channelId string) (ok bool, err error) {
 	transaction, err := s.GetMasterX().Beginx()
 	if err != nil {
@@ -291,17 +293,20 @@ func (s SqlSharedChannelStore) Delete(channelId string) (ok bool, err error) {
 		return false, errors.Wrap(err, "failed to delete SharedChannel")
 	}
 
+	curTime := model.GetMillis()
+
 	// Also remove remotes from SharedChannelRemotes (if any).
 	squery, args, err = s.getQueryBuilder().
-		Delete("SharedChannelRemotes").
+		Update("SharedChannelRemotes").
+		Set("UpdateAt", curTime).
+		Set("DeleteAt", curTime).
 		Where(sq.Eq{"ChannelId": channelId}).
 		ToSql()
 	if err != nil {
 		return false, errors.Wrap(err, "delete_shared_channel_remotes_tosql")
 	}
 
-	_, err = transaction.Exec(squery, args...)
-	if err != nil {
+	if _, err = transaction.Exec(squery, args...); err != nil {
 		return false, errors.Wrap(err, "failed to delete SharedChannelRemotes")
 	}
 
@@ -337,10 +342,10 @@ func (s SqlSharedChannelStore) SaveRemote(remote *model.SharedChannelRemote) (*m
 	}
 
 	query, args, err := s.getQueryBuilder().Insert("SharedChannelRemotes").
-		Columns("Id", "ChannelId", "CreatorId", "CreateAt", "UpdateAt", "IsInviteAccepted", "IsInviteConfirmed", "RemoteId",
+		Columns("Id", "ChannelId", "CreatorId", "CreateAt", "UpdateAt", "DeleteAt", "IsInviteAccepted", "IsInviteConfirmed", "RemoteId",
 			"LastPostCreateAt", "LastPostCreateId", "LastPostUpdateAt", "LastPostId").
-		Values(remote.Id, remote.ChannelId, remote.CreatorId, remote.CreateAt, remote.UpdateAt, remote.IsInviteAccepted, remote.IsInviteConfirmed, remote.RemoteId,
-			remote.LastPostCreateAt, remote.LastPostCreateID, remote.LastPostUpdateAt, remote.LastPostUpdateID).
+		Values(remote.Id, remote.ChannelId, remote.CreatorId, remote.CreateAt, remote.UpdateAt, remote.DeleteAt, remote.IsInviteAccepted, remote.IsInviteConfirmed,
+			remote.RemoteId, remote.LastPostCreateAt, remote.LastPostCreateID, remote.LastPostUpdateAt, remote.LastPostUpdateID).
 		ToSql()
 	if err != nil {
 		return nil, errors.Wrapf(err, "savesharedchannelremote_tosql")
@@ -362,6 +367,7 @@ func (s SqlSharedChannelStore) UpdateRemote(remote *model.SharedChannelRemote) (
 		Set("CreatorId", remote.CreatorId).
 		Set("CreateAt", remote.CreateAt).
 		Set("UpdateAt", remote.UpdateAt).
+		Set("DeleteAt", remote.DeleteAt).
 		Set("IsInviteAccepted", remote.IsInviteAccepted).
 		Set("IsInviteConfirmed", remote.IsInviteConfirmed).
 		Set("RemoteId", remote.RemoteId).
@@ -403,6 +409,7 @@ func sharedChannelRemoteFields(prefix string) []string {
 		prefix + "CreatorId",
 		prefix + "CreateAt",
 		prefix + "UpdateAt",
+		prefix + "DeleteAt",
 		prefix + "IsInviteAccepted",
 		prefix + "IsInviteConfirmed",
 		prefix + "RemoteId",
@@ -504,6 +511,10 @@ func (s SqlSharedChannelStore) GetRemotes(offset, limit int, opts model.SharedCh
 
 	query = query.Offset(uint64(offset)).Limit(uint64(limit))
 
+	if !opts.IncludeDeleted {
+		query = query.Where(sq.Eq{"DeleteAt": 0})
+	}
+
 	squery, args, err := query.ToSql()
 	if err != nil {
 		return nil, errors.Wrapf(err, "get_shared_channel_remotes_tosql")
@@ -526,6 +537,7 @@ func (s SqlSharedChannelStore) HasRemote(channelID string, remoteId string) (boo
 		From("SharedChannelRemotes").
 		Where(sq.Eq{"RemoteId": remoteId}).
 		Where(sq.Eq{"ChannelId": channelID}).
+		Where(sq.Eq{"DeleteAt": 0}).
 		Suffix(")")
 
 	query, args, err := builder.ToSql()
@@ -549,6 +561,7 @@ func (s SqlSharedChannelStore) GetRemoteForUser(remoteId string, userId string)
 		Join("SharedChannelRemotes AS scr ON rc.RemoteId = scr.RemoteId").
 		Join("ChannelMembers AS cm ON scr.ChannelId = cm.ChannelId").
 		Where(sq.Eq{"rc.RemoteId": remoteId}).
+		Where(sq.Eq{"scr.DeleteAt": 0}).
 		Where(sq.Eq{"cm.UserId": userId})
 
 	query, args, err := builder.ToSql()
@@ -614,8 +627,12 @@ func (s SqlSharedChannelStore) UpdateRemoteCursor(id string, cursor model.GetPos
 // DeleteRemote deletes a single shared channel remote.
 // Returns true if remote found and deleted, false if not found.
 func (s SqlSharedChannelStore) DeleteRemote(id string) (bool, error) {
+	curTime := model.GetMillis()
+
 	squery, args, err := s.getQueryBuilder().
-		Delete("SharedChannelRemotes").
+		Update("SharedChannelRemotes").
+		Set("DeleteAt", curTime).
+		Set("UpdateAt", curTime).
 		Where(sq.Eq{"Id": id}).
 		ToSql()
 	if err != nil {
@@ -644,6 +661,7 @@ func (s SqlSharedChannelStore) GetRemotesStatus(channelId string) ([]*model.Shar
 		Select("scr.ChannelId, rc.DisplayName, rc.SiteURL, rc.LastPingAt, sc.ReadOnly, scr.IsInviteAccepted").
 		From("SharedChannelRemotes scr, RemoteClusters rc, SharedChannels sc").
 		Where("scr.RemoteId = rc.RemoteId").
+		Where("scr.DeleteAt = 0").
 		Where("scr.ChannelId = sc.ChannelId").
 		Where(sq.Eq{"scr.ChannelId": channelId})
 

server/channels/store/storetest/remote_cluster_store.go

@@ -145,7 +145,7 @@ func testRemoteClusterSave(t *testing.T, _ request.CTX, ss store.Store) {
 	})
 }
 
-func testRemoteClusterDelete(t *testing.T, _ request.CTX, ss store.Store) {
+func testRemoteClusterDelete(t *testing.T, rctx request.CTX, ss store.Store) {
 	t.Run("Delete", func(t *testing.T) {
 		rc := &model.RemoteCluster{
 			Name:      "shortlived_remote",
@@ -158,6 +158,57 @@ func testRemoteClusterDelete(t *testing.T, _ request.CTX, ss store.Store) {
 		deleted, err := ss.RemoteCluster().Delete(rcSaved.RemoteId)
 		require.NoError(t, err)
 		require.True(t, deleted)
+
+		deletedRC, err := ss.RemoteCluster().Get(rcSaved.RemoteId)
+		require.NoError(t, err)
+		require.NotZero(t, deletedRC.DeleteAt)
+	})
+
+	t.Run("Delete with shared channel remotes", func(t *testing.T) {
+		rc := &model.RemoteCluster{
+			Name:      "shortlived_remote",
+			SiteURL:   makeSiteURL(),
+			CreatorId: model.NewId(),
+		}
+		rcSaved, err := ss.RemoteCluster().Save(rc)
+		require.NoError(t, err)
+
+		// we create a shared channel remote for the remote cluster
+		channel, err := createTestChannel(ss, rctx, "test_delete")
+		require.NoError(t, err)
+
+		sc := &model.SharedChannel{
+			ChannelId: channel.Id,
+			TeamId:    channel.TeamId,
+			CreatorId: model.NewId(),
+			ShareName: "testshare",
+			RemoteId:  model.NewId(),
+		}
+
+		_, err = ss.SharedChannel().Save(sc)
+		require.NoError(t, err, "couldn't save shared channel", err)
+
+		scr := &model.SharedChannelRemote{
+			ChannelId: channel.Id,
+			CreatorId: model.NewId(),
+			RemoteId:  rc.RemoteId,
+		}
+		scrSaved, err := ss.SharedChannel().SaveRemote(scr)
+		require.NoError(t, err)
+
+		// and then we delete the cluster, expecting the shared
+		// channel remote to be deleted as well
+		deleted, err := ss.RemoteCluster().Delete(rcSaved.RemoteId)
+		require.NoError(t, err)
+		require.True(t, deleted)
+
+		deletedRC, err := ss.RemoteCluster().Get(rcSaved.RemoteId)
+		require.NoError(t, err)
+		require.NotZero(t, deletedRC.DeleteAt)
+
+		deletedSCR, err := ss.SharedChannel().GetRemote(scrSaved.Id)
+		require.NoError(t, err)
+		require.NotZero(t, deletedSCR.DeleteAt)
 	})
 
 	t.Run("Delete nonexistent", func(t *testing.T) {
@@ -218,7 +269,7 @@ func testRemoteClusterGetByPluginID(t *testing.T, _ request.CTX, ss store.Store)
 }
 
 func testRemoteClusterGetAll(t *testing.T, _ request.CTX, ss store.Store) {
-	require.NoError(t, clearRemoteClusters(ss))
+	ss.DropAllTables()
 
 	userId := model.NewId()
 	now := model.GetMillis()
@@ -232,9 +283,11 @@ func testRemoteClusterGetAll(t *testing.T, _ request.CTX, ss store.Store) {
 		{Name: "brand_new_offline_remote", CreatorId: userId, SiteURL: "", LastPingAt: 0, Topics: " bogus shared stuff "},
 		{Name: "offline_plugin_remote", CreatorId: model.NewId(), SiteURL: makeSiteURL(), PluginID: model.NewId(), LastPingAt: 0, Topics: " pluginshare "},
 		{Name: "online_plugin_remote", CreatorId: model.NewId(), SiteURL: makeSiteURL(), PluginID: model.NewId(), LastPingAt: now, Topics: " pluginshare "},
+		{Name: "deleted_remote", CreatorId: model.NewId(), SiteURL: "", LastPingAt: 0, DeleteAt: 123},
 	}
 
 	idsAll := make([]string, 0)
+	idsNotDeleted := make([]string, 0)
 	idsOnline := make([]string, 0)
 	idsShareTopic := make([]string, 0)
 	idsPlugin := make([]string, 0)
@@ -246,24 +299,29 @@ func testRemoteClusterGetAll(t *testing.T, _ request.CTX, ss store.Store) {
 		saved, err := ss.RemoteCluster().Save(item)
 		require.NoError(t, err)
 		idsAll = append(idsAll, saved.RemoteId)
-		if online {
-			idsOnline = append(idsOnline, saved.RemoteId)
-		}
-		if strings.Contains(saved.Topics, " shared ") {
-			idsShareTopic = append(idsShareTopic, saved.RemoteId)
-		}
-		if item.PluginID != "" {
-			idsPlugin = append(idsPlugin, saved.RemoteId)
-		} else {
-			idsNotPlugin = append(idsNotPlugin, saved.RemoteId)
-		}
-		if item.SiteURL != "" {
-			idsConfirmed = append(idsConfirmed, saved.RemoteId)
+		if item.DeleteAt == 0 {
+			idsNotDeleted = append(idsNotDeleted, saved.RemoteId)
+
+			// only include non-deleted items in other counts
+			if online {
+				idsOnline = append(idsOnline, saved.RemoteId)
+			}
+			if strings.Contains(saved.Topics, " shared ") {
+				idsShareTopic = append(idsShareTopic, saved.RemoteId)
+			}
+			if item.PluginID != "" {
+				idsPlugin = append(idsPlugin, saved.RemoteId)
+			} else {
+				idsNotPlugin = append(idsNotPlugin, saved.RemoteId)
+			}
+			if item.SiteURL != "" {
+				idsConfirmed = append(idsConfirmed, saved.RemoteId)
+			}
 		}
 	}
 
 	t.Run("GetAll", func(t *testing.T) {
-		filter := model.RemoteClusterQueryFilter{}
+		filter := model.RemoteClusterQueryFilter{IncludeDeleted: true}
 		remotes, err := ss.RemoteCluster().GetAll(0, 999999, filter)
 		require.NoError(t, err)
 		// make sure all the test data remotes were returned.
@@ -271,6 +329,15 @@ func testRemoteClusterGetAll(t *testing.T, _ request.CTX, ss store.Store) {
 		assert.ElementsMatch(t, ids, idsAll)
 	})
 
+	t.Run("GetAllNotDeleted", func(t *testing.T) {
+		filter := model.RemoteClusterQueryFilter{}
+		remotes, err := ss.RemoteCluster().GetAll(0, 999999, filter)
+		require.NoError(t, err)
+		// make sure all the test data remotes were returned.
+		ids := getIds(remotes)
+		assert.ElementsMatch(t, ids, idsNotDeleted)
+	})
+
 	t.Run("GetAll online only", func(t *testing.T) {
 		filter := model.RemoteClusterQueryFilter{
 			ExcludeOffline: true,
@@ -375,7 +442,7 @@ func testRemoteClusterGetAllInChannel(t *testing.T, rctx request.CTX, ss store.S
 		testPluginID_2 = "com.sample.bloop"
 	)
 
-	require.NoError(t, clearRemoteClusters(ss))
+	ss.DropAllTables()
 	now := model.GetMillis()
 
 	userId := model.NewId()
@@ -485,7 +552,7 @@ func testRemoteClusterGetAllInChannel(t *testing.T, rctx request.CTX, ss store.S
 }
 
 func testRemoteClusterGetAllNotInChannel(t *testing.T, rctx request.CTX, ss store.Store) {
-	require.NoError(t, clearRemoteClusters(ss))
+	ss.DropAllTables()
 
 	userId := model.NewId()
 
@@ -590,7 +657,7 @@ func getIds(remotes []*model.RemoteCluster) []string {
 }
 
 func testRemoteClusterGetByTopic(t *testing.T, _ request.CTX, ss store.Store) {
-	require.NoError(t, clearRemoteClusters(ss))
+	ss.DropAllTables()
 
 	rcData := []*model.RemoteCluster{
 		{Name: "AAAA_Inc", CreatorId: model.NewId(), SiteURL: "aaaa.com", RemoteId: model.NewId(), Topics: ""},
@@ -670,17 +737,3 @@ func testRemoteClusterUpdateTopics(t *testing.T, _ request.CTX, ss store.Store)
 		require.Equal(t, tt.expected, rcUpdated.Topics)
 	}
 }
-
-func clearRemoteClusters(ss store.Store) error {
-	list, err := ss.RemoteCluster().GetAll(0, 999999, model.RemoteClusterQueryFilter{})
-	if err != nil {
-		return err
-	}
-
-	for _, rc := range list {
-		if _, err := ss.RemoteCluster().Delete(rc.RemoteId); err != nil {
-			return err
-		}
-	}
-	return nil
-}

server/channels/store/storetest/shared_channel_store.go

@@ -390,9 +390,10 @@ func testDeleteSharedChannel(t *testing.T, rctx request.CTX, ss store.Store) {
 	// add some remotes
 	for i := 0; i < 10; i++ {
 		remote := &model.SharedChannelRemote{
-			ChannelId: channel.Id,
-			CreatorId: model.NewId(),
-			RemoteId:  model.NewId(),
+			ChannelId:         channel.Id,
+			CreatorId:         model.NewId(),
+			RemoteId:          model.NewId(),
+			IsInviteConfirmed: true, // to avoid adding the InclUnconfirmed filter
 		}
 		_, err := ss.SharedChannel().SaveRemote(remote)
 		require.NoError(t, err, "couldn't add remote", err)
@@ -407,10 +408,14 @@ func testDeleteSharedChannel(t *testing.T, rctx request.CTX, ss store.Store) {
 		require.Error(t, err)
 		require.Nil(t, sc)
 
-		// make sure the remotes were deleted.
+		// make sure the remotes were marked as deleted.
 		remotes, err := ss.SharedChannel().GetRemotes(0, 999999, model.SharedChannelRemoteFilterOpts{ChannelId: channel.Id})
 		require.NoError(t, err)
-		require.Len(t, remotes, 0, "expected empty remotes list")
+		require.Len(t, remotes, 0)
+
+		deletedRemotes, err := ss.SharedChannel().GetRemotes(0, 999999, model.SharedChannelRemoteFilterOpts{ChannelId: channel.Id, IncludeDeleted: true})
+		require.NoError(t, err)
+		require.Len(t, deletedRemotes, 10)
 
 		// ensure channel's Shared flag is unset
 		channelMod, err := ss.Channel().Get(channel.Id, false)
@@ -571,6 +576,17 @@ func testGetSharedChannelRemoteByIds(t *testing.T, rctx request.CTX, ss store.St
 		require.Error(t, err)
 		require.Nil(t, r)
 	})
+
+	t.Run("Get deleted shared channel remote by ids", func(t *testing.T) {
+		deleted, err := ss.SharedChannel().DeleteRemote(remoteSaved.Id)
+		require.NoError(t, err)
+		require.True(t, deleted)
+
+		r, err := ss.SharedChannel().GetRemoteByIds(remoteSaved.ChannelId, remoteSaved.RemoteId)
+		require.NoError(t, err)
+		require.Equal(t, remoteSaved.Id, r.Id)
+		require.NotZero(t, r.DeleteAt)
+	})
 }
 
 func testGetSharedChannelRemotes(t *testing.T, rctx request.CTX, ss store.Store) {
@@ -587,6 +603,7 @@ func testGetSharedChannelRemotes(t *testing.T, rctx request.CTX, ss store.Store)
 		{ChannelId: channel.Id, CreatorId: creator, RemoteId: remoteId2, IsInviteConfirmed: true},
 		{CreatorId: creator, RemoteId: remoteId, IsInviteConfirmed: true},
 		{CreatorId: creator, RemoteId: remoteId, IsInviteConfirmed: true},
+		{CreatorId: creator, RemoteId: remoteId, IsInviteConfirmed: true, DeleteAt: 123},
 		{CreatorId: creator, RemoteId: remoteId},
 	}
 
@@ -641,6 +658,25 @@ func testGetSharedChannelRemotes(t *testing.T, rctx request.CTX, ss store.Store)
 		}
 	})
 
+	t.Run("Get shared channel remotes by remote_id including deleted", func(t *testing.T) {
+		opts := model.SharedChannelRemoteFilterOpts{
+			RemoteId:       remoteId,
+			IncludeDeleted: true,
+		}
+		remotes, err := ss.SharedChannel().GetRemotes(0, 999999, opts)
+		require.NoError(t, err, "should not error", err)
+		require.Len(t, remotes, 3) // only confirmed invitations
+		deletedCount := 0
+		for _, r := range remotes {
+			require.Equal(t, remoteId, r.RemoteId)
+			require.True(t, r.IsInviteConfirmed)
+			if r.DeleteAt != 0 {
+				deletedCount++
+			}
+		}
+		require.Equal(t, 1, deletedCount)
+	})
+
 	t.Run("Get shared channel remotes by invalid remote_id", func(t *testing.T) {
 		opts := model.SharedChannelRemoteFilterOpts{
 			RemoteId: model.NewId(),
@@ -751,6 +787,20 @@ func testHasRemote(t *testing.T, rctx request.CTX, ss store.Store) {
 		require.NoError(t, err)
 		assert.False(t, has)
 	})
+
+	t.Run("deleted remote", func(t *testing.T) {
+		scr, err := ss.SharedChannel().GetRemoteByIds(channel.Id, remote1)
+		require.NoError(t, err)
+		require.NotEmpty(t, scr.Id)
+
+		deleted, err := ss.SharedChannel().DeleteRemote(scr.Id)
+		require.NoError(t, err)
+		require.True(t, deleted)
+
+		has, err := ss.SharedChannel().HasRemote(channel.Id, remote1)
+		require.NoError(t, err)
+		assert.False(t, has)
+	})
 }
 
 func testGetRemoteForUser(t *testing.T, rctx request.CTX, ss store.Store) {
@@ -888,8 +938,8 @@ func testDeleteSharedChannelRemote(t *testing.T, rctx request.CTX, ss store.Stor
 		require.True(t, deleted, "expected true from delete remote")
 
 		r, err := ss.SharedChannel().GetRemote(remoteSaved.Id)
-		require.Error(t, err)
-		require.Nil(t, r)
+		require.NoError(t, err)
+		require.NotZero(t, r.DeleteAt)
 	})
 
 	t.Run("Delete non-existent shared channel remote", func(t *testing.T) {