From 30cb527cf606fc271acbbe27560646ffb24c474a Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Fri, 18 Oct 2024 19:16:29 +0200 Subject: [PATCH] Forbid users from creating DMs and GMs with remote users (#28855) * Forbid users from creating DMs and GMs with remote users This change prevents the application from creating new DMs and GMs when remote users are involved, and prevents as well new posts from being created into preexisting DMs and GMs with remote users created in previous versions. * Adds i18n * Skip preexisting DM tests and create a new one for the new behavior --- server/channels/api4/shared_channel_test.go | 26 ++++++ server/channels/app/channel.go | 10 +++ server/channels/app/channel_test.go | 33 ++++++++ server/channels/app/post.go | 4 + server/channels/app/post_test.go | 79 +++++++++++++++++++ server/i18n/en.json | 12 +++ .../services/sharedchannel/service_api.go | 4 + 7 files changed, 168 insertions(+) diff --git a/server/channels/api4/shared_channel_test.go b/server/channels/api4/shared_channel_test.go index cecb507c36..729af1f42b 100644 --- a/server/channels/api4/shared_channel_test.go +++ b/server/channels/api4/shared_channel_test.go @@ -161,7 +161,29 @@ func TestGetRemoteClusterById(t *testing.T) { } func TestCreateDirectChannelWithRemoteUser(t *testing.T) { + t.Run("should not create a local DM channel that is shared", func(t *testing.T) { + th := setupForSharedChannels(t).InitBasic() + defer th.TearDown() + client := th.Client + defer func() { + _, err := client.Logout(context.Background()) + require.NoError(t, err) + }() + + localUser := th.BasicUser + remoteUser := th.CreateUser() + remoteUser.RemoteId = model.NewPointer(model.NewId()) + remoteUser, appErr := th.App.UpdateUser(th.Context, remoteUser, false) + require.Nil(t, appErr) + + dm, _, err := client.CreateDirectChannel(context.Background(), localUser.Id, remoteUser.Id) + require.Error(t, err) + require.Nil(t, dm) + }) + t.Run("creates a local DM channel that is shared", func(t *testing.T) { + t.Skip("Remote DMs are currently disabled") + th := setupForSharedChannels(t).InitBasic() defer th.TearDown() client := th.Client @@ -185,6 +207,8 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) { }) t.Run("sends a shared channel invitation to the remote", func(t *testing.T) { + t.Skip("Remote DMs are currently disabled") + th := setupForSharedChannels(t).InitBasic() defer th.TearDown() client := th.Client @@ -217,6 +241,8 @@ func TestCreateDirectChannelWithRemoteUser(t *testing.T) { }) t.Run("does not send a shared channel invitation to the remote when creator is remote", func(t *testing.T) { + t.Skip("Remote DMs are currently disabled") + th := setupForSharedChannels(t).InitBasic() defer th.TearDown() client := th.Client diff --git a/server/channels/app/channel.go b/server/channels/app/channel.go index 6e433bb35c..2571b3adc7 100644 --- a/server/channels/app/channel.go +++ b/server/channels/app/channel.go @@ -424,6 +424,10 @@ func (a *App) createDirectChannel(c request.CTX, userID string, otherUserID stri } func (a *App) createDirectChannelWithUser(c request.CTX, user, otherUser *model.User, channelOptions ...model.ChannelOption) (*model.Channel, *model.AppError) { + if user.IsRemote() || otherUser.IsRemote() { + return nil, model.NewAppError("createDirectChannelWithUser", "api.channel.create_channel.direct_channel.remote_restricted.app_error", nil, "", http.StatusForbidden) + } + channel, nErr := a.Srv().Store().Channel().CreateDirectChannel(c, user, otherUser, channelOptions...) if nErr != nil { var invErr *store.ErrInvalidInput @@ -524,6 +528,12 @@ func (a *App) createGroupChannel(c request.CTX, userIDs []string) (*model.Channe return nil, model.NewAppError("CreateGroupChannel", "api.channel.create_group.bad_user.app_error", nil, "user_ids="+model.ArrayToJSON(userIDs), http.StatusBadRequest) } + for _, user := range users { + if user.IsRemote() { + return nil, model.NewAppError("createGroupChannel", "api.channel.create_group.remote_restricted.app_error", nil, "", http.StatusForbidden) + } + } + group := &model.Channel{ Name: model.GetGroupNameFromUserIds(userIDs), DisplayName: model.GetGroupDisplayNameFromUsers(users, true), diff --git a/server/channels/app/channel_test.go b/server/channels/app/channel_test.go index 0294c26405..317a4aabfb 100644 --- a/server/channels/app/channel_test.go +++ b/server/channels/app/channel_test.go @@ -471,6 +471,39 @@ func TestGetOrCreateDirectChannel(t *testing.T) { require.Nil(t, channel, "channel should be nil") require.NotNil(t, appErr) }) + + t.Run("Cannot create with a remote user", func(t *testing.T) { + user2.RemoteId = model.NewPointer(model.NewId()) + _, appErr := th.App.UpdateUser(th.Context, user2, false) + require.Nil(t, appErr) + + dm, appErr := th.App.GetOrCreateDirectChannel(th.Context, user1.Id, user2.Id) + require.Nil(t, dm) + require.NotNil(t, appErr) + }) +} + +func TestCreateGroupChannel(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + user1 := th.CreateUser() + user2 := th.CreateUser() + + groupUserIds := make([]string, 0) + groupUserIds = append(groupUserIds, user1.Id) + groupUserIds = append(groupUserIds, user2.Id) + groupUserIds = append(groupUserIds, th.BasicUser.Id) + + t.Run("Should not allow to create a group with a remote user", func(t *testing.T) { + user2.RemoteId = model.NewPointer(model.NewId()) + _, appErr := th.App.UpdateUser(th.Context, user2, false) + require.Nil(t, appErr) + + dm, appErr := th.App.CreateGroupChannel(th.Context, groupUserIds, th.BasicUser.Id) + require.NotNil(t, appErr) + require.Nil(t, dm) + }) } func TestCreateGroupChannelCreatesChannelMemberHistoryRecord(t *testing.T) { diff --git a/server/channels/app/post.go b/server/channels/app/post.go index 4a254ad988..6ab4dec56e 100644 --- a/server/channels/app/post.go +++ b/server/channels/app/post.go @@ -141,6 +141,10 @@ func (a *App) deduplicateCreatePost(rctx request.CTX, post *model.Post) (foundPo } func (a *App) CreatePost(c request.CTX, post *model.Post, channel *model.Channel, triggerWebhooks, setOnline bool) (savedPost *model.Post, err *model.AppError) { + if channel.IsShared() && (channel.Type == model.ChannelTypeDirect || channel.Type == model.ChannelTypeGroup) { + return nil, model.NewAppError("CreatePost", "app.post.create_post.shared_dm_or_gm.app_error", nil, "", http.StatusBadRequest) + } + foundPost, err := a.deduplicateCreatePost(c, post) if err != nil { return nil, err diff --git a/server/channels/app/post_test.go b/server/channels/app/post_test.go index 5fdd6cd8f0..7e09b90ccf 100644 --- a/server/channels/app/post_test.go +++ b/server/channels/app/post_test.go @@ -1050,6 +1050,85 @@ func TestCreatePost(t *testing.T) { } }) + t.Run("Should not allow to create posts on shared DMs", func(t *testing.T) { + th := setupSharedChannels(t).InitBasic() + defer th.TearDown() + + user1 := th.CreateUser() + user2 := th.CreateUser() + dm, err := th.App.createDirectChannel(th.Context, user1.Id, user2.Id) + require.Nil(t, err) + require.NotNil(t, dm) + + // we can't create direct channels with remote users, so we + // have to force the channel to be shared through the store to + // simulate preexisting shared DMs + sc := &model.SharedChannel{ + ChannelId: dm.Id, + Type: dm.Type, + Home: true, + ShareName: "shareddm", + CreatorId: user1.Id, + RemoteId: model.NewId(), + } + _, scErr := th.Server.Store().SharedChannel().Save(sc) + require.NoError(t, scErr) + + // and we update the channel to mark it as shared + dm.Shared = model.NewPointer(true) + _, cErr := th.Server.Store().Channel().Update(th.Context, dm) + require.NoError(t, cErr) + + newPost := &model.Post{ + ChannelId: dm.Id, + Message: "hello world", + UserId: user1.Id, + } + createdPost, err := th.App.CreatePost(th.Context, newPost, dm, false, false) + require.NotNil(t, err) + require.Nil(t, createdPost) + }) + + t.Run("Should not allow to create posts on shared GMs", func(t *testing.T) { + th := setupSharedChannels(t).InitBasic() + defer th.TearDown() + + user1 := th.CreateUser() + user2 := th.CreateUser() + user3 := th.CreateUser() + gm, gErr := th.App.createGroupChannel(th.Context, []string{user1.Id, user2.Id, user3.Id}) + require.Nil(t, gErr) + require.NotNil(t, gm) + + // we can't create group channels with remote users, so we + // have to force the channel to be shared through the store to + // simulate preexisting shared GMs + sc := &model.SharedChannel{ + ChannelId: gm.Id, + Type: gm.Type, + Home: true, + ShareName: "sharedgm", + CreatorId: user1.Id, + RemoteId: model.NewId(), + } + _, scErr := th.Server.Store().SharedChannel().Save(sc) + require.NoError(t, scErr) + + // and we update the channel to mark it as shared + gm.Shared = model.NewPointer(true) + _, cErr := th.Server.Store().Channel().Update(th.Context, gm) + require.NoError(t, cErr) + + newPost := &model.Post{ + ChannelId: gm.Id, + Message: "hello world", + UserId: user1.Id, + } + createdPost, err := th.App.CreatePost(th.Context, newPost, gm, false, false) + require.NotNil(t, err) + require.Nil(t, createdPost) + }) + t.Run("MM-40016 should not panic with `concurrent map read and map write`", func(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() diff --git a/server/i18n/en.json b/server/i18n/en.json index fa24a131bb..406c708ad0 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -299,6 +299,10 @@ "id": "api.channel.create_channel.direct_channel.app_error", "translation": "Must use createDirectChannel API service for direct message channel creation." }, + { + "id": "api.channel.create_channel.direct_channel.remote_restricted.app_error", + "translation": "Cannot create a direct channel with a remote user" + }, { "id": "api.channel.create_channel.direct_channel.team_restricted_error", "translation": "A direct channel cannot be created between these users because they do not share a team in common." @@ -335,6 +339,10 @@ "id": "api.channel.create_group.bad_user.app_error", "translation": "One of the provided users does not exist." }, + { + "id": "api.channel.create_group.remote_restricted.app_error", + "translation": "Cannot create a group channel with remote users" + }, { "id": "api.channel.delete_channel.archived", "translation": "%v archived the channel." @@ -6154,6 +6162,10 @@ "id": "app.post.cloud.get.app_error", "translation": "Can not fetch the post as it is past the cloud's plan limit." }, + { + "id": "app.post.create_post.shared_dm_or_gm.app_error", + "translation": "Cannot create the post in a DM or GM with remote users" + }, { "id": "app.post.delete.app_error", "translation": "Unable to delete the post." diff --git a/server/platform/services/sharedchannel/service_api.go b/server/platform/services/sharedchannel/service_api.go index b1d1804e06..3fbc0a8148 100644 --- a/server/platform/services/sharedchannel/service_api.go +++ b/server/platform/services/sharedchannel/service_api.go @@ -22,6 +22,10 @@ func (scs *Service) ShareChannel(sc *model.SharedChannel) (*model.SharedChannel, return nil, fmt.Errorf("cannot fetch channel while sharing channel %s: %w", sc.ChannelId, err) } + if channel.Type == model.ChannelTypeDirect || channel.Type == model.ChannelTypeGroup { + return nil, errors.New("cannot share a direct or group channel") + } + // check if channel is already shared scExisting, err := scs.server.GetStore().SharedChannel().Get(sc.ChannelId) if err == nil {