MM-57867 Don't delete existing channel on invitation error (#27143)

* Do not delete existing channels
* don't allow resharung

server/platform/services/sharedchannel/channelinvite.go

@@ -54,7 +54,7 @@ func (scs *Service) SendChannelInvite(channel *model.Channel, userId string, rc
 		ChannelId:   channel.Id,
 		TeamId:      rc.RemoteTeamId,
 		ReadOnly:    sc.ReadOnly,
-		Name:        sc.ShareName,
+		Name:        channel.Name,
 		DisplayName: sc.ShareDisplayName,
 		Header:      sc.ShareHeader,
 		Purpose:     sc.SharePurpose,
@@ -162,14 +162,21 @@ func (scs *Service) onReceiveChannelInvite(msg model.RemoteClusterMsg, rc *model
 		mlog.String("team_id", invite.TeamId),
 	)
 
-	// create channel if it doesn't exist; the channel may already exist, such as if it was shared then unshared at some point.
-	channel, err := scs.server.GetStore().Channel().Get(invite.ChannelId, true)
-	if err != nil {
-		if channel, err = scs.handleChannelCreation(invite, rc); err != nil {
-			return err
-		}
+	// check if channel already exists
+	var channel *model.Channel
+	_, err := scs.server.GetStore().Channel().Get(invite.ChannelId, true)
+	if err == nil {
+		// the channel already exists on this server; could be the remote is trying to re-share it (not allowed at this time).
+		// If the channel is already shared with the remote, it will remain so.
+		return fmt.Errorf("cannot create shared channel (channel_id=%s): %w", invite.ChannelId, model.ErrChannelAlreadyExists)
 	}
 
+	// create new local channel to sync with the remote channel
+	if channel, err = scs.handleChannelCreation(invite, rc); err != nil {
+		return err
+	}
+
+	// mark the newly created channel read-only if requested in the invite
 	if invite.ReadOnly {
 		if err := scs.makeChannelReadOnly(channel); err != nil {
 			return fmt.Errorf("cannot make channel readonly `%s`: %w", invite.ChannelId, err)
@@ -191,6 +198,7 @@ func (scs *Service) onReceiveChannelInvite(msg model.RemoteClusterMsg, rc *model
 	}
 
 	if _, err := scs.server.GetStore().SharedChannel().Save(sharedChannel); err != nil {
+		// delete the newly created channel since we could not create a SharedChannel record for it
 		scs.app.PermanentDeleteChannel(request.EmptyContext(scs.server.Log()), channel)
 		return fmt.Errorf("cannot create shared channel (channel_id=%s): %w", invite.ChannelId, err)
 	}
@@ -207,6 +215,8 @@ func (scs *Service) onReceiveChannelInvite(msg model.RemoteClusterMsg, rc *model
 	}
 
 	if _, err := scs.server.GetStore().SharedChannel().SaveRemote(sharedChannelRemote); err != nil {
+		// delete the newly created channel since we could not create a SharedChannelRemote record for it,
+		// and delete the newly created SharedChannel record as well.
 		scs.app.PermanentDeleteChannel(request.EmptyContext(scs.server.Log()), channel)
 		scs.server.GetStore().SharedChannel().Delete(sharedChannel.ChannelId)
 		return fmt.Errorf("cannot create shared channel remote (channel_id=%s): %w", invite.ChannelId, err)

server/platform/services/sharedchannel/channelinvite_test.go

@@ -16,9 +16,16 @@ import (
 	"github.com/mattermost/mattermost/server/public/model"
 	"github.com/mattermost/mattermost/server/public/plugin/plugintest/mock"
 	"github.com/mattermost/mattermost/server/public/shared/mlog"
+	"github.com/mattermost/mattermost/server/v8/channels/store"
 	"github.com/mattermost/mattermost/server/v8/channels/store/storetest/mocks"
 )
 
+var (
+	mockTypeChannel    = mock.AnythingOfType("*model.Channel")
+	mockTypeString     = mock.AnythingOfType("string")
+	mockTypeReqContext = mock.AnythingOfType("*request.Context")
+)
+
 func TestOnReceiveChannelInvite(t *testing.T) {
 	t.Run("when msg payload is empty, it does nothing", func(t *testing.T) {
 		mockServer := &MockServerIface{}
@@ -58,7 +65,7 @@ func TestOnReceiveChannelInvite(t *testing.T) {
 			ChannelId: model.NewId(),
 			TeamId:    model.NewId(),
 			ReadOnly:  true,
-			Type:      "0",
+			Type:      model.ChannelTypeOpen,
 		}
 		payload, err := json.Marshal(invitation)
 		require.NoError(t, err)
@@ -68,15 +75,18 @@ func TestOnReceiveChannelInvite(t *testing.T) {
 		}
 		mockChannelStore := mocks.ChannelStore{}
 		mockSharedChannelStore := mocks.SharedChannelStore{}
-		channel := &model.Channel{}
+		channel := &model.Channel{
+			Id:     invitation.ChannelId,
+			TeamId: invitation.TeamId,
+			Type:   invitation.Type,
+		}
 
-		mockChannelStore.On("Get", invitation.ChannelId, true).Return(channel, nil)
+		mockChannelStore.On("Get", invitation.ChannelId, true).Return(nil, &store.ErrNotFound{})
 		mockSharedChannelStore.On("Save", mock.Anything).Return(nil, nil)
 		mockSharedChannelStore.On("SaveRemote", mock.Anything).Return(nil, nil)
 		mockStore.On("Channel").Return(&mockChannelStore)
 		mockStore.On("SharedChannel").Return(&mockSharedChannelStore)
 
-		mockServer = scs.server.(*MockServerIface)
 		mockServer.On("GetStore").Return(mockStore)
 		createPostPermission := model.ChannelModeratedPermissionsMap[model.PermissionCreatePost.Id]
 		createReactionPermission := model.ChannelModeratedPermissionsMap[model.PermissionAddReaction.Id]
@@ -85,6 +95,8 @@ func TestOnReceiveChannelInvite(t *testing.T) {
 			Members: model.NewBool(false),
 		}
 
+		mockApp.On("CreateChannelWithUser", mockTypeReqContext, mockTypeChannel, mockTypeString).Return(channel, nil)
+
 		readonlyChannelModerations := []*model.ChannelModerationPatch{
 			{
 				Name:  &createPostPermission,
@@ -95,7 +107,7 @@ func TestOnReceiveChannelInvite(t *testing.T) {
 				Roles: &updateMap,
 			},
 		}
-		mockApp.On("PatchChannelModerationsForChannel", mock.Anything, channel, readonlyChannelModerations).Return(nil, nil)
+		mockApp.On("PatchChannelModerationsForChannel", mock.Anything, channel, readonlyChannelModerations).Return(nil, nil).Maybe()
 		defer mockApp.AssertExpectations(t)
 
 		err = scs.onReceiveChannelInvite(msg, remoteCluster, nil)
@@ -129,13 +141,14 @@ func TestOnReceiveChannelInvite(t *testing.T) {
 		mockChannelStore := mocks.ChannelStore{}
 		channel := &model.Channel{}
 
-		mockChannelStore.On("Get", invitation.ChannelId, true).Return(channel, nil)
+		mockChannelStore.On("Get", invitation.ChannelId, true).Return(nil, &store.ErrNotFound{})
 		mockStore.On("Channel").Return(&mockChannelStore)
 
 		mockServer = scs.server.(*MockServerIface)
 		mockServer.On("GetStore").Return(mockStore)
 		appErr := model.NewAppError("foo", "bar", nil, "boom", http.StatusBadRequest)
 
+		mockApp.On("CreateChannelWithUser", mockTypeReqContext, mockTypeChannel, mockTypeString).Return(channel, nil)
 		mockApp.On("PatchChannelModerationsForChannel", mock.Anything, channel, mock.Anything).Return(nil, appErr)
 		defer mockApp.AssertExpectations(t)
 

server/public/model/shared_channel.go

@@ -14,6 +14,7 @@ import (
 var (
 	ErrChannelAlreadyShared = errors.New("channel is already shared")
 	ErrChannelHomedOnRemote = errors.New("channel is homed on a remote cluster")
+	ErrChannelAlreadyExists = errors.New("channel already exists")
 )
 
 // SharedChannel represents a channel that can be synchronized with a remote cluster.