From d490fdc1d9f5fce76c4e290adf96e283140780d1 Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Sat, 22 Jun 2024 23:08:55 +0200 Subject: [PATCH] Marks the RemoteTeamId field of RemoteClusters as deprecated (#27364) * Marks the RemoteTeamId field of RemoteClusters as deprecated The `RemoteTeamId` was used both in the `RemoteCluster` model and as part of remote invites. It existed so two different remotes could have multiple secure connections between them, and have each of those connections scoped to a team, sharing through each only the channels that belong to their corresponding team. The way that we're thinking on the feature currently only contemplates one secure connection between two servers, and shares all the channels through that secure connection, so this field is no longer needed. As we don't have a system in place for the user to choose in which team a channel should be created from an invite, this change adds a mechanism that checks the invite for a teamId, and if it's not present, fetches a team from the database to create the channel into. This makes the change backwards compatible for secure connections that already have an established behavior and allows us to move forward with the implementation of an alternative. * Mark invite teamId field as deprecated --------- Co-authored-by: Mattermost Build --- server/channels/api4/shared_channel_test.go | 9 +- server/channels/app/remote_cluster_test.go | 113 ++++++------------ server/channels/app/session_test.go | 9 +- .../app/slashcommands/command_remote.go | 7 +- .../store/storetest/shared_channel_store.go | 7 +- .../services/remotecluster/invitation.go | 22 ++-- .../platform/services/remotecluster/recv.go | 1 - .../services/sharedchannel/channelinvite.go | 17 ++- server/public/model/auditconv.go | 1 - server/public/model/remote_cluster.go | 4 +- server/public/model/remote_cluster_test.go | 7 +- 11 files changed, 79 insertions(+), 118 deletions(-) diff --git a/server/channels/api4/shared_channel_test.go b/server/channels/api4/shared_channel_test.go index 96f50733cc..7ec223b078 100644 --- a/server/channels/api4/shared_channel_test.go +++ b/server/channels/api4/shared_channel_test.go @@ -108,11 +108,10 @@ func TestGetRemoteClusterById(t *testing.T) { // create a remote cluster rc := &model.RemoteCluster{ - RemoteId: model.NewId(), - Name: "Test1", - RemoteTeamId: model.NewId(), - SiteURL: model.NewId(), - CreatorId: model.NewId(), + RemoteId: model.NewId(), + Name: "Test1", + SiteURL: model.NewId(), + CreatorId: model.NewId(), } rc, appErr := th.App.AddRemoteCluster(rc) require.Nil(t, appErr) diff --git a/server/channels/app/remote_cluster_test.go b/server/channels/app/remote_cluster_test.go index d49ad488bc..3a98355d6c 100644 --- a/server/channels/app/remote_cluster_test.go +++ b/server/channels/app/remote_cluster_test.go @@ -23,15 +23,14 @@ func TestAddRemoteCluster(t *testing.T) { th := setupRemoteCluster(t).InitBasic() defer th.TearDown() - t.Run("adding remote cluster with duplicate site url and remote team id", func(t *testing.T) { + t.Run("adding remote cluster with duplicate site url", func(t *testing.T) { remoteCluster := &model.RemoteCluster{ - RemoteTeamId: model.NewId(), - Name: "test1", - SiteURL: "http://www1.example.com:8065", - Token: model.NewId(), - RemoteToken: model.NewId(), - Topics: "", - CreatorId: th.BasicUser.Id, + Name: "test1", + SiteURL: "http://www1.example.com:8065", + Token: model.NewId(), + RemoteToken: model.NewId(), + Topics: "", + CreatorId: th.BasicUser.Id, } _, err := th.App.AddRemoteCluster(remoteCluster) @@ -42,60 +41,29 @@ func TestAddRemoteCluster(t *testing.T) { 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")) }) - - t.Run("adding remote cluster with duplicate site url or remote team id is allowed", func(t *testing.T) { - remoteCluster := &model.RemoteCluster{ - RemoteTeamId: model.NewId(), - Name: "test2", - SiteURL: "http://www2.example.com:8065", - Token: model.NewId(), - RemoteToken: model.NewId(), - Topics: "", - CreatorId: th.BasicUser.Id, - } - - existingRemoteCluster, err := th.App.AddRemoteCluster(remoteCluster) - require.Nil(t, err, "Adding a remote cluster should not error") - - // Same site url but different remote team id - remoteCluster.RemoteId = model.NewId() - remoteCluster.RemoteTeamId = model.NewId() - remoteCluster.SiteURL = existingRemoteCluster.SiteURL - _, err = th.App.AddRemoteCluster(remoteCluster) - assert.Nil(t, err, "Adding a remote cluster should not error") - - // Same remote team id but different site url - remoteCluster.RemoteId = model.NewId() - remoteCluster.RemoteTeamId = existingRemoteCluster.RemoteTeamId - remoteCluster.SiteURL = existingRemoteCluster.SiteURL + "/new" - _, err = th.App.AddRemoteCluster(remoteCluster) - assert.Nil(t, err, "Adding a remote cluster should not error") - }) } func TestUpdateRemoteCluster(t *testing.T) { th := setupRemoteCluster(t).InitBasic() defer th.TearDown() - t.Run("update remote cluster with an already existing site url and team id", func(t *testing.T) { + t.Run("update remote cluster with an already existing site url", func(t *testing.T) { remoteCluster := &model.RemoteCluster{ - RemoteTeamId: model.NewId(), - Name: "test3", - SiteURL: "http://www3.example.com:8065", - Token: model.NewId(), - RemoteToken: model.NewId(), - Topics: "", - CreatorId: th.BasicUser.Id, + Name: "test3", + SiteURL: "http://www3.example.com:8065", + Token: model.NewId(), + RemoteToken: model.NewId(), + Topics: "", + CreatorId: th.BasicUser.Id, } otherRemoteCluster := &model.RemoteCluster{ - RemoteTeamId: model.NewId(), - Name: "test4", - SiteURL: "http://www4.example.com:8066", - Token: model.NewId(), - RemoteToken: model.NewId(), - Topics: "", - CreatorId: th.BasicUser.Id, + Name: "test4", + SiteURL: "http://www4.example.com:8066", + Token: model.NewId(), + RemoteToken: model.NewId(), + Topics: "", + CreatorId: th.BasicUser.Id, } _, err := th.App.AddRemoteCluster(remoteCluster) @@ -105,31 +73,28 @@ func TestUpdateRemoteCluster(t *testing.T) { require.Nil(t, err, "Adding a remote cluster should not error") savedRemoteClustered.SiteURL = remoteCluster.SiteURL - savedRemoteClustered.RemoteTeamId = remoteCluster.RemoteTeamId _, 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")) }) - t.Run("update remote cluster with an already existing site url or team id, is allowed", func(t *testing.T) { + t.Run("update remote cluster with an already existing site url, is not allowed", func(t *testing.T) { remoteCluster := &model.RemoteCluster{ - RemoteTeamId: model.NewId(), - Name: "test5", - SiteURL: "http://www5.example.com:8065", - Token: model.NewId(), - RemoteToken: model.NewId(), - Topics: "", - CreatorId: th.BasicUser.Id, + Name: "test5", + SiteURL: "http://www5.example.com:8065", + Token: model.NewId(), + RemoteToken: model.NewId(), + Topics: "", + CreatorId: th.BasicUser.Id, } otherRemoteCluster := &model.RemoteCluster{ - RemoteTeamId: model.NewId(), - Name: "test6", - SiteURL: "http://www6.example.com:8065", - Token: model.NewId(), - RemoteToken: model.NewId(), - Topics: "", - CreatorId: th.BasicUser.Id, + Name: "test6", + SiteURL: "http://www6.example.com:8065", + Token: model.NewId(), + RemoteToken: model.NewId(), + Topics: "", + CreatorId: th.BasicUser.Id, } existingRemoteCluster, err := th.App.AddRemoteCluster(remoteCluster) @@ -138,16 +103,10 @@ func TestUpdateRemoteCluster(t *testing.T) { anotherExistingRemoteClustered, err := th.App.AddRemoteCluster(otherRemoteCluster) require.Nil(t, err, "Adding a remote cluster should not error") - // Same site url but different remote team id + // Same site url anotherExistingRemoteClustered.SiteURL = existingRemoteCluster.SiteURL - anotherExistingRemoteClustered.RemoteTeamId = model.NewId() _, err = th.App.UpdateRemoteCluster(anotherExistingRemoteClustered) - assert.Nil(t, err, "Updating remote cluster should not error") - - // Same remote team id but different site url - anotherExistingRemoteClustered.SiteURL = existingRemoteCluster.SiteURL + "/new" - anotherExistingRemoteClustered.RemoteTeamId = existingRemoteCluster.RemoteTeamId - _, err = th.App.UpdateRemoteCluster(anotherExistingRemoteClustered) - assert.Nil(t, err, "Updating remote cluster should not error") + require.NotNil(t, err, "Updating remote cluster should error") + assert.Contains(t, err.Error(), i18n.T("api.remote_cluster.update_not_unique.app_error")) }) } diff --git a/server/channels/app/session_test.go b/server/channels/app/session_test.go index 0556a16cc4..46958b16de 100644 --- a/server/channels/app/session_test.go +++ b/server/channels/app/session_test.go @@ -370,11 +370,10 @@ func TestGetRemoteClusterSession(t *testing.T) { remoteID := model.NewId() rc := model.RemoteCluster{ - RemoteId: remoteID, - RemoteTeamId: model.NewId(), - Name: "test", - Token: token, - CreatorId: model.NewId(), + RemoteId: remoteID, + Name: "test", + Token: token, + CreatorId: model.NewId(), } _, err := th.GetSqlStore().RemoteCluster().Save(&rc) diff --git a/server/channels/app/slashcommands/command_remote.go b/server/channels/app/slashcommands/command_remote.go index cca5adc571..bbe55508b7 100644 --- a/server/channels/app/slashcommands/command_remote.go +++ b/server/channels/app/slashcommands/command_remote.go @@ -145,10 +145,9 @@ func (rp *RemoteProvider) doCreate(a *app.App, args *model.CommandArgs, margs ma // Display the encrypted invitation invite := &model.RemoteClusterInvite{ - RemoteId: rcSaved.RemoteId, - RemoteTeamId: args.TeamId, - SiteURL: url, - Token: rcSaved.Token, + RemoteId: rcSaved.RemoteId, + SiteURL: url, + Token: rcSaved.Token, } encrypted, err := invite.Encrypt(password) if err != nil { diff --git a/server/channels/store/storetest/shared_channel_store.go b/server/channels/store/storetest/shared_channel_store.go index 9cf0073a60..24213b89e4 100644 --- a/server/channels/store/storetest/shared_channel_store.go +++ b/server/channels/store/storetest/shared_channel_store.go @@ -698,13 +698,12 @@ func testHasRemote(t *testing.T, rctx request.CTX, ss store.Store) { func testGetRemoteForUser(t *testing.T, rctx request.CTX, ss store.Store) { // add remotes, and users to simulated shared channels. - teamId := model.NewId() channel, err := createSharedTestChannel(ss, rctx, "share_test_channel", true, nil) require.NoError(t, err) remotes := []*model.RemoteCluster{ - {RemoteId: model.NewId(), SiteURL: model.NewId(), CreatorId: model.NewId(), RemoteTeamId: teamId, Name: "Test_Remote_1"}, - {RemoteId: model.NewId(), SiteURL: model.NewId(), CreatorId: model.NewId(), RemoteTeamId: teamId, Name: "Test_Remote_2"}, - {RemoteId: model.NewId(), SiteURL: model.NewId(), CreatorId: model.NewId(), RemoteTeamId: teamId, Name: "Test_Remote_3"}, + {RemoteId: model.NewId(), SiteURL: model.NewId(), CreatorId: model.NewId(), Name: "Test_Remote_1"}, + {RemoteId: model.NewId(), SiteURL: model.NewId(), CreatorId: model.NewId(), Name: "Test_Remote_2"}, + {RemoteId: model.NewId(), SiteURL: model.NewId(), CreatorId: model.NewId(), Name: "Test_Remote_3"}, } for _, rc := range remotes { _, err := ss.RemoteCluster().Save(rc) diff --git a/server/platform/services/remotecluster/invitation.go b/server/platform/services/remotecluster/invitation.go index aec044b522..7c44d5ebc9 100644 --- a/server/platform/services/remotecluster/invitation.go +++ b/server/platform/services/remotecluster/invitation.go @@ -14,14 +14,13 @@ import ( // AcceptInvitation is called when accepting an invitation to connect with a remote cluster. func (rcs *Service) AcceptInvitation(invite *model.RemoteClusterInvite, name string, displayName, creatorId string, teamId string, siteURL string) (*model.RemoteCluster, error) { rc := &model.RemoteCluster{ - RemoteId: invite.RemoteId, - RemoteTeamId: invite.RemoteTeamId, - Name: name, - DisplayName: displayName, - Token: model.NewId(), - RemoteToken: invite.Token, - SiteURL: invite.SiteURL, - CreatorId: creatorId, + RemoteId: invite.RemoteId, + Name: name, + DisplayName: displayName, + Token: model.NewId(), + RemoteToken: invite.Token, + SiteURL: invite.SiteURL, + CreatorId: creatorId, } rcSaved, err := rcs.server.GetStore().RemoteCluster().Save(rc) @@ -63,10 +62,9 @@ func (rcs *Service) AcceptInvitation(invite *model.RemoteClusterInvite, name str func makeConfirmFrame(rc *model.RemoteCluster, teamId string, siteURL string) (*model.RemoteClusterFrame, error) { confirm := model.RemoteClusterInvite{ - RemoteId: rc.RemoteId, - RemoteTeamId: teamId, - SiteURL: siteURL, - Token: rc.Token, + RemoteId: rc.RemoteId, + SiteURL: siteURL, + Token: rc.Token, } confirmRaw, err := json.Marshal(confirm) if err != nil { diff --git a/server/platform/services/remotecluster/recv.go b/server/platform/services/remotecluster/recv.go index 77eef686fc..a132dfbd20 100644 --- a/server/platform/services/remotecluster/recv.go +++ b/server/platform/services/remotecluster/recv.go @@ -62,7 +62,6 @@ func (rcs *Service) ReceiveInviteConfirmation(confirm model.RemoteClusterInvite) return nil, fmt.Errorf("cannot accept invite confirmation for remote %s: %w", confirm.RemoteId, err) } - rc.RemoteTeamId = confirm.RemoteTeamId rc.SiteURL = confirm.SiteURL rc.RemoteToken = confirm.Token diff --git a/server/platform/services/sharedchannel/channelinvite.go b/server/platform/services/sharedchannel/channelinvite.go index 6063dd47b1..592b40d2d0 100644 --- a/server/platform/services/sharedchannel/channelinvite.go +++ b/server/platform/services/sharedchannel/channelinvite.go @@ -18,7 +18,7 @@ import ( // channelInviteMsg represents an invitation for a remote cluster to start sharing a channel. type channelInviteMsg struct { ChannelId string `json:"channel_id"` - TeamId string `json:"team_id"` + TeamId string `json:"team_id"` // Deprecated: this field is no longer used. It's only kept for backwards compatibility. ReadOnly bool `json:"read_only"` Name string `json:"name"` DisplayName string `json:"display_name"` @@ -52,7 +52,6 @@ func (scs *Service) SendChannelInvite(channel *model.Channel, userId string, rc invite := channelInviteMsg{ ChannelId: channel.Id, - TeamId: rc.RemoteTeamId, ReadOnly: sc.ReadOnly, Name: channel.Name, DisplayName: sc.ShareDisplayName, @@ -256,9 +255,21 @@ func (scs *Service) handleChannelCreation(invite channelInviteMsg, rc *model.Rem return scs.createDirectChannel(invite, rc) } + teamId := invite.TeamId + // if the invite doesn't have a teamId associated and until the + // acceptance of an invite includes selecting a team, we use the + // first team of the list + if teamId == "" { + teams, err := scs.server.GetStore().Team().GetAllPage(0, 1, nil) + if err != nil { + return nil, fmt.Errorf("cannot get team to create the channel `%s`: %w", invite.ChannelId, err) + } + teamId = teams[0].Id + } + channelNew := &model.Channel{ Id: invite.ChannelId, - TeamId: invite.TeamId, + TeamId: teamId, Type: invite.Type, DisplayName: invite.DisplayName, Name: invite.Name, diff --git a/server/public/model/auditconv.go b/server/public/model/auditconv.go index b3182beee7..be7ac04a94 100644 --- a/server/public/model/auditconv.go +++ b/server/public/model/auditconv.go @@ -750,7 +750,6 @@ func newRemoteCluster(r *RemoteCluster) auditRemoteCluster { var rc auditRemoteCluster if r != nil { rc.RemoteId = r.RemoteId - rc.RemoteTeamId = r.RemoteTeamId rc.Name = r.Name rc.DisplayName = r.DisplayName rc.SiteURL = r.SiteURL diff --git a/server/public/model/remote_cluster.go b/server/public/model/remote_cluster.go index 3b8986145c..92700f0daa 100644 --- a/server/public/model/remote_cluster.go +++ b/server/public/model/remote_cluster.go @@ -51,7 +51,7 @@ func (bm *Bitmask) UnsetBit(flag Bitmask) { type RemoteCluster struct { RemoteId string `json:"remote_id"` - RemoteTeamId string `json:"remote_team_id"` + RemoteTeamId string `json:"remote_team_id"` // Deprecated: this field is no longer used. It's only kept for backwards compatibility. Name string `json:"name"` DisplayName string `json:"display_name"` SiteURL string `json:"site_url"` @@ -312,7 +312,7 @@ type RemoteClusterPing struct { // RemoteClusterInvite represents an invitation to establish a simple trust with a remote cluster. type RemoteClusterInvite struct { RemoteId string `json:"remote_id"` - RemoteTeamId string `json:"remote_team_id"` + RemoteTeamId string `json:"remote_team_id"` // Deprecated: this field is no longer used. It's only kept for backwards compatibility. SiteURL string `json:"site_url"` Token string `json:"token"` } diff --git a/server/public/model/remote_cluster_test.go b/server/public/model/remote_cluster_test.go index 21be8e4dba..294b25ff11 100644 --- a/server/public/model/remote_cluster_test.go +++ b/server/public/model/remote_cluster_test.go @@ -130,10 +130,9 @@ func TestRemoteClusterInviteEncryption(t *testing.T) { func makeInvite(url string) RemoteClusterInvite { return RemoteClusterInvite{ - RemoteId: NewId(), - RemoteTeamId: NewId(), - SiteURL: url, - Token: NewId(), + RemoteId: NewId(), + SiteURL: url, + Token: NewId(), } }