From 59873a2a5067454670c5199ab9e433e2396c8fb7 Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Thu, 8 Aug 2024 16:36:27 +0200 Subject: [PATCH] Adds default team to the remote cluster entity (#27863) * Adds default team to the remote cluster entity A new DefaultTeamId field is added to the RemoteCluster entity and its endpoints, and used when receiving channel invites to choose in which team to create a new channel. This will be later extended with the ability for the system admin to manually accept invites, choosing which team to create the channel on each. This use case will be triggered when the DefaultTeamId field is empty, which now simply chooses the first team it finds in the database as a fallback. * Fix migrations list * Fixes channelinvite test case * Fix i18n * Fix migration list --- api/v4/source/definitions.yaml | 3 + api/v4/source/remoteclusters.yaml | 3 + server/channels/api4/remote_cluster.go | 11 ++-- server/channels/api4/remote_cluster_test.go | 17 +++-- server/channels/db/migrations/migrations.list | 4 ++ ...emoteclusters_add_default_team_id.down.sql | 1 + ..._remoteclusters_add_default_team_id.up.sql | 14 +++++ ...emoteclusters_add_default_team_id.down.sql | 1 + ..._remoteclusters_add_default_team_id.up.sql | 1 + .../store/sqlstore/remote_cluster_store.go | 6 +- .../services/sharedchannel/channelinvite.go | 4 +- .../sharedchannel/channelinvite_test.go | 8 ++- server/public/model/remote_cluster.go | 63 +++++++++++-------- server/public/model/remote_cluster_test.go | 2 + 14 files changed, 99 insertions(+), 39 deletions(-) create mode 100644 server/channels/db/migrations/mysql/000125_remoteclusters_add_default_team_id.down.sql create mode 100644 server/channels/db/migrations/mysql/000125_remoteclusters_add_default_team_id.up.sql create mode 100644 server/channels/db/migrations/postgres/000125_remoteclusters_add_default_team_id.down.sql create mode 100644 server/channels/db/migrations/postgres/000125_remoteclusters_add_default_team_id.up.sql diff --git a/api/v4/source/definitions.yaml b/api/v4/source/definitions.yaml index 77c9c6db0d..44fc458ee6 100644 --- a/api/v4/source/definitions.yaml +++ b/api/v4/source/definitions.yaml @@ -3499,6 +3499,9 @@ components: site_url: description: URL of the remote cluster type: string + default_team_id: + description: The team where channels from invites are created + type: string create_at: description: Time in milliseconds that the remote cluster was created type: integer diff --git a/api/v4/source/remoteclusters.yaml b/api/v4/source/remoteclusters.yaml index 25c1ec068b..a3852b6e6f 100644 --- a/api/v4/source/remoteclusters.yaml +++ b/api/v4/source/remoteclusters.yaml @@ -171,6 +171,9 @@ properties: display_name: type: string + default_team_id: + type: string + description: The team where channels from invites are created responses: "200": description: Remote Cluster patch successful diff --git a/server/channels/api4/remote_cluster.go b/server/channels/api4/remote_cluster.go index 2ecb10dd11..8360e45287 100644 --- a/server/channels/api4/remote_cluster.go +++ b/server/channels/api4/remote_cluster.go @@ -378,11 +378,12 @@ func createRemoteCluster(c *Context, w http.ResponseWriter, r *http.Request) { } rc := &model.RemoteCluster{ - Name: rcWithTeamAndPassword.Name, - DisplayName: rcWithTeamAndPassword.DisplayName, - SiteURL: model.SiteURLPending + model.NewId(), - Token: model.NewId(), - CreatorId: c.AppContext.Session().UserId, + Name: rcWithTeamAndPassword.Name, + DisplayName: rcWithTeamAndPassword.DisplayName, + SiteURL: model.SiteURLPending + model.NewId(), + DefaultTeamId: rcWithTeamAndPassword.DefaultTeamId, + Token: model.NewId(), + CreatorId: c.AppContext.Session().UserId, } audit.AddEventParameterAuditable(auditRec, "remotecluster", rc) diff --git a/server/channels/api4/remote_cluster_test.go b/server/channels/api4/remote_cluster_test.go index 1bbe3bbbbf..4a6606152f 100644 --- a/server/channels/api4/remote_cluster_test.go +++ b/server/channels/api4/remote_cluster_test.go @@ -158,9 +158,10 @@ func TestGetRemoteClusters(t *testing.T) { func TestCreateRemoteCluster(t *testing.T) { rcWithTeamAndPassword := &model.RemoteClusterWithPassword{ RemoteCluster: &model.RemoteCluster{ - Name: "remotecluster", - SiteURL: "http://example.com", - Token: model.NewId(), + Name: "remotecluster", + SiteURL: "http://example.com", + DefaultTeamId: model.NewId(), + Token: model.NewId(), }, Password: "mysupersecret", } @@ -231,6 +232,7 @@ func TestCreateRemoteCluster(t *testing.T) { CheckCreatedStatus(t, resp) require.NoError(t, err) require.Equal(t, rcWithTeamAndPassword.Name, rcWithInvite.RemoteCluster.Name) + require.Equal(t, rcWithTeamAndPassword.DefaultTeamId, rcWithInvite.RemoteCluster.DefaultTeamId) require.NotZero(t, rcWithInvite.Invite) require.Zero(t, rcWithInvite.RemoteCluster.Token) require.Zero(t, rcWithInvite.RemoteCluster.RemoteToken) @@ -431,6 +433,7 @@ func TestGetRemoteCluster(t *testing.T) { defer th.TearDown() newRC.CreatorId = th.SystemAdminUser.Id + newRC.DefaultTeamId = th.BasicTeam.Id rc, appErr := th.App.AddRemoteCluster(newRC) require.Nil(t, appErr) @@ -455,6 +458,7 @@ func TestGetRemoteCluster(t *testing.T) { CheckOKStatus(t, resp) require.NoError(t, err) require.Equal(t, rc.RemoteId, fetchedRC.RemoteId) + require.Equal(t, th.BasicTeam.Id, fetchedRC.DefaultTeamId) require.Empty(t, fetchedRC.Token) }) } @@ -509,12 +513,17 @@ func TestPatchRemoteCluster(t *testing.T) { }) t.Run("should correctly patch the remote cluster", func(t *testing.T) { - rcp := &model.RemoteClusterPatch{DisplayName: model.NewPointer("patched!")} + newTeamId := model.NewId() + rcp := &model.RemoteClusterPatch{ + DisplayName: model.NewPointer("patched!"), + DefaultTeamId: model.NewPointer(newTeamId), + } patchedRC, resp, err := th.SystemAdminClient.PatchRemoteCluster(context.Background(), rc.RemoteId, rcp) CheckOKStatus(t, resp) require.NoError(t, err) require.Equal(t, "patched!", patchedRC.DisplayName) + require.Equal(t, newTeamId, patchedRC.DefaultTeamId) }) } diff --git a/server/channels/db/migrations/migrations.list b/server/channels/db/migrations/migrations.list index 1566f48a74..6a890f3cc1 100644 --- a/server/channels/db/migrations/migrations.list +++ b/server/channels/db/migrations/migrations.list @@ -246,6 +246,8 @@ channels/db/migrations/mysql/000123_remove_upload_file_permission.down.sql channels/db/migrations/mysql/000123_remove_upload_file_permission.up.sql 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/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 @@ -492,3 +494,5 @@ channels/db/migrations/postgres/000123_remove_upload_file_permission.down.sql channels/db/migrations/postgres/000123_remove_upload_file_permission.up.sql 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 diff --git a/server/channels/db/migrations/mysql/000125_remoteclusters_add_default_team_id.down.sql b/server/channels/db/migrations/mysql/000125_remoteclusters_add_default_team_id.down.sql new file mode 100644 index 0000000000..7e9ccde63d --- /dev/null +++ b/server/channels/db/migrations/mysql/000125_remoteclusters_add_default_team_id.down.sql @@ -0,0 +1 @@ +-- Skipping it because the forward migrations are destructive \ No newline at end of file diff --git a/server/channels/db/migrations/mysql/000125_remoteclusters_add_default_team_id.up.sql b/server/channels/db/migrations/mysql/000125_remoteclusters_add_default_team_id.up.sql new file mode 100644 index 0000000000..0219ab7aac --- /dev/null +++ b/server/channels/db/migrations/mysql/000125_remoteclusters_add_default_team_id.up.sql @@ -0,0 +1,14 @@ +SET @preparedStatement = (SELECT IF( + ( + SELECT COUNT(*) FROM INFORMATION_SCHEMA.COLUMNS + WHERE table_name = 'RemoteClusters' + AND table_schema = DATABASE() + AND column_name = 'DefaultTeamId' + ) > 0, + 'SELECT 1', + 'ALTER TABLE RemoteClusters ADD DefaultTeamId VARCHAR(26) DEFAULT "";' +)); + +PREPARE alterIfNotExists FROM @preparedStatement; +EXECUTE alterIfNotExists; +DEALLOCATE PREPARE alterIfNotExists; diff --git a/server/channels/db/migrations/postgres/000125_remoteclusters_add_default_team_id.down.sql b/server/channels/db/migrations/postgres/000125_remoteclusters_add_default_team_id.down.sql new file mode 100644 index 0000000000..7e9ccde63d --- /dev/null +++ b/server/channels/db/migrations/postgres/000125_remoteclusters_add_default_team_id.down.sql @@ -0,0 +1 @@ +-- Skipping it because the forward migrations are destructive \ No newline at end of file diff --git a/server/channels/db/migrations/postgres/000125_remoteclusters_add_default_team_id.up.sql b/server/channels/db/migrations/postgres/000125_remoteclusters_add_default_team_id.up.sql new file mode 100644 index 0000000000..e3044395bf --- /dev/null +++ b/server/channels/db/migrations/postgres/000125_remoteclusters_add_default_team_id.up.sql @@ -0,0 +1 @@ +ALTER TABLE remoteclusters ADD COLUMN IF NOT EXISTS defaultteamid character varying(26) DEFAULT ''; diff --git a/server/channels/store/sqlstore/remote_cluster_store.go b/server/channels/store/sqlstore/remote_cluster_store.go index 2994eeceff..4f3864af27 100644 --- a/server/channels/store/sqlstore/remote_cluster_store.go +++ b/server/channels/store/sqlstore/remote_cluster_store.go @@ -34,6 +34,7 @@ func remoteClusterFields(prefix string) []string { prefix + "Name", prefix + "DisplayName", prefix + "SiteURL", + prefix + "DefaultTeamId", prefix + "CreateAt", prefix + "LastPingAt", prefix + "Token", @@ -65,10 +66,10 @@ func (s sqlRemoteClusterStore) Save(remoteCluster *model.RemoteCluster) (*model. } query := `INSERT INTO RemoteClusters - (RemoteId, RemoteTeamId, Name, DisplayName, SiteURL, CreateAt, + (RemoteId, RemoteTeamId, Name, DisplayName, SiteURL, DefaultTeamId, CreateAt, LastPingAt, Token, RemoteToken, Topics, CreatorId, PluginID, Options) VALUES - (:RemoteId, :RemoteTeamId, :Name, :DisplayName, :SiteURL, :CreateAt, + (:RemoteId, :RemoteTeamId, :Name, :DisplayName, :SiteURL, :DefaultTeamId, :CreateAt, :LastPingAt, :Token, :RemoteToken, :Topics, :CreatorId, :PluginID, :Options)` if _, err := s.GetMasterX().NamedExec(query, remoteCluster); err != nil { @@ -93,6 +94,7 @@ func (s sqlRemoteClusterStore) Update(remoteCluster *model.RemoteCluster) (*mode CreatorId = :CreatorId, DisplayName = :DisplayName, SiteURL = :SiteURL, + DefaultTeamId = :DefaultTeamId, Topics = :Topics, PluginID = :PluginID, Options = :Options diff --git a/server/platform/services/sharedchannel/channelinvite.go b/server/platform/services/sharedchannel/channelinvite.go index d4499c946f..53505aca95 100644 --- a/server/platform/services/sharedchannel/channelinvite.go +++ b/server/platform/services/sharedchannel/channelinvite.go @@ -255,8 +255,8 @@ 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 + teamId := rc.DefaultTeamId + // if the remote 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 == "" { diff --git a/server/platform/services/sharedchannel/channelinvite_test.go b/server/platform/services/sharedchannel/channelinvite_test.go index 14ae9a45e1..063e2d7b35 100644 --- a/server/platform/services/sharedchannel/channelinvite_test.go +++ b/server/platform/services/sharedchannel/channelinvite_test.go @@ -62,7 +62,7 @@ func TestOnReceiveChannelInvite(t *testing.T) { } mockStore := &mocks.Store{} - remoteCluster := &model.RemoteCluster{Name: "test"} + remoteCluster := &model.RemoteCluster{Name: "test", DefaultTeamId: model.NewId()} invitation := channelInviteMsg{ ChannelId: model.NewId(), TeamId: model.NewId(), @@ -144,9 +144,15 @@ func TestOnReceiveChannelInvite(t *testing.T) { channel := &model.Channel{ Id: invitation.ChannelId, } + mockTeamStore := mocks.TeamStore{} + team := &model.Team{ + Id: model.NewId(), + } mockChannelStore.On("Get", invitation.ChannelId, true).Return(nil, &store.ErrNotFound{}) + mockTeamStore.On("GetAllPage", 0, 1, mock.Anything).Return([]*model.Team{team}, nil) mockStore.On("Channel").Return(&mockChannelStore) + mockStore.On("Team").Return(&mockTeamStore) mockServer = scs.server.(*MockServerIface) mockServer.On("GetStore").Return(mockStore) diff --git a/server/public/model/remote_cluster.go b/server/public/model/remote_cluster.go index 590a22b757..25959dd025 100644 --- a/server/public/model/remote_cluster.go +++ b/server/public/model/remote_cluster.go @@ -50,33 +50,35 @@ func (bm *Bitmask) UnsetBit(flag Bitmask) { } type RemoteCluster struct { - RemoteId string `json:"remote_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"` - CreateAt int64 `json:"create_at"` - LastPingAt int64 `json:"last_ping_at"` - Token string `json:"token"` - RemoteToken string `json:"remote_token"` - Topics string `json:"topics"` - CreatorId string `json:"creator_id"` - PluginID string `json:"plugin_id"` // non-empty when sync message are to be delivered via plugin API - Options Bitmask `json:"options"` // bit-flag set of options + RemoteId string `json:"remote_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"` + DefaultTeamId string `json:"default_team_id"` + CreateAt int64 `json:"create_at"` + LastPingAt int64 `json:"last_ping_at"` + Token string `json:"token"` + RemoteToken string `json:"remote_token"` + Topics string `json:"topics"` + CreatorId string `json:"creator_id"` + PluginID string `json:"plugin_id"` // non-empty when sync message are to be delivered via plugin API + Options Bitmask `json:"options"` // bit-flag set of options } func (rc *RemoteCluster) Auditable() map[string]interface{} { return map[string]interface{}{ - "remote_id": rc.RemoteId, - "remote_team_id": rc.RemoteTeamId, - "name": rc.Name, - "display_name": rc.DisplayName, - "site_url": rc.SiteURL, - "create_at": rc.CreateAt, - "last_ping_at": rc.LastPingAt, - "creator_id": rc.CreatorId, - "plugin_id": rc.PluginID, - "options": rc.Options, + "remote_id": rc.RemoteId, + "remote_team_id": rc.RemoteTeamId, + "name": rc.Name, + "display_name": rc.DisplayName, + "site_url": rc.SiteURL, + "default_team_id": rc.DefaultTeamId, + "create_at": rc.CreateAt, + "last_ping_at": rc.LastPingAt, + "creator_id": rc.CreatorId, + "plugin_id": rc.PluginID, + "options": rc.Options, } } @@ -123,6 +125,11 @@ func (rc *RemoteCluster) IsValid() *AppError { if !IsValidId(rc.CreatorId) { return NewAppError("RemoteCluster.IsValid", "model.cluster.is_valid.id.app_error", nil, "creator_id="+rc.CreatorId, http.StatusBadRequest) } + + if rc.DefaultTeamId != "" && !IsValidId(rc.DefaultTeamId) { + return NewAppError("RemoteCluster.IsValid", "model.cluster.is_valid.id.app_error", nil, "default_team_id="+rc.DefaultTeamId, http.StatusBadRequest) + } + return nil } @@ -132,12 +139,14 @@ func (rc *RemoteCluster) Sanitize() { } type RemoteClusterPatch struct { - DisplayName *string `json:"display_name"` + DisplayName *string `json:"display_name"` + DefaultTeamId *string `json:"default_team_id"` } func (rcp *RemoteClusterPatch) Auditable() map[string]interface{} { return map[string]interface{}{ - "display_name": rcp.DisplayName, + "display_name": rcp.DisplayName, + "default_team_id": rcp.DefaultTeamId, } } @@ -145,6 +154,10 @@ func (rc *RemoteCluster) Patch(patch *RemoteClusterPatch) { if patch.DisplayName != nil { rc.DisplayName = *patch.DisplayName } + + if patch.DefaultTeamId != nil { + rc.DefaultTeamId = *patch.DefaultTeamId + } } type RemoteClusterWithPassword struct { diff --git a/server/public/model/remote_cluster_test.go b/server/public/model/remote_cluster_test.go index 294b25ff11..cdb1019d19 100644 --- a/server/public/model/remote_cluster_test.go +++ b/server/public/model/remote_cluster_test.go @@ -27,6 +27,8 @@ func TestRemoteClusterIsValid(t *testing.T) { {name: "Missing create_at", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "example.com"}, valid: false}, {name: "Missing last_ping_at", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "example.com", CreatorId: creator, CreateAt: now}, valid: true}, {name: "Missing creator", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "example.com", CreateAt: now, LastPingAt: now}, valid: false}, + {name: "Bad default_team_id", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "example.com", CreateAt: now, LastPingAt: now, CreatorId: creator, DefaultTeamId: "bad-id"}, valid: false}, + {name: "Valid default_team_id", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "example.com", CreateAt: now, LastPingAt: now, CreatorId: creator, DefaultTeamId: NewId()}, valid: true}, {name: "RemoteCluster valid", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "example.com", CreateAt: now, LastPingAt: now, CreatorId: creator}, valid: true}, {name: "Include protocol", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "http://example.com", CreateAt: now, LastPingAt: now, CreatorId: creator}, valid: true}, {name: "Include protocol & port", rc: &RemoteCluster{RemoteId: id, Name: NewId(), SiteURL: "http://example.com:8065", CreateAt: now, LastPingAt: now, CreatorId: creator}, valid: true},