From 923f6a594384a9c96ee36d34509f835a4025dc37 Mon Sep 17 00:00:00 2001 From: Doug Lauder Date: Wed, 29 May 2024 00:46:18 -0400 Subject: [PATCH] MM-57867 Don't delete existing channel on invitation error (#27143) * Do not delete existing channels * don't allow resharung --- .../services/sharedchannel/channelinvite.go | 24 ++++++++++++------ .../sharedchannel/channelinvite_test.go | 25 ++++++++++++++----- server/public/model/shared_channel.go | 1 + 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/server/platform/services/sharedchannel/channelinvite.go b/server/platform/services/sharedchannel/channelinvite.go index d0e1d22c1c..977eeb397d 100644 --- a/server/platform/services/sharedchannel/channelinvite.go +++ b/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) diff --git a/server/platform/services/sharedchannel/channelinvite_test.go b/server/platform/services/sharedchannel/channelinvite_test.go index d5d2c6b10a..293c56bbab 100644 --- a/server/platform/services/sharedchannel/channelinvite_test.go +++ b/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) diff --git a/server/public/model/shared_channel.go b/server/public/model/shared_channel.go index a920f16059..63632b93c1 100644 --- a/server/public/model/shared_channel.go +++ b/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.