From d448a6bef30339a63b7c118b8b2deb44cdb79d8e Mon Sep 17 00:00:00 2001 From: Jonathan Date: Wed, 7 Mar 2018 10:54:51 -0500 Subject: XYZ-87: GlobalRelay DM Exports do not Include Channel Name (#8275) * Adding ChannelMemberHistory table records when DM channels are created. This ensures that both participants in a DM are shown in compliance export, even if only one of them typed anything * Direct/Group Message channels now get pretty display names for the purpose of compliance exports * Fixed string formatting in t.Fatal calls * Changed uses of ChannelMemberHistory over to ChannelMemberHistoryResult in tests. This should have been done as a part of the work in XYZ-110, but seems to have been missed in this branch for some reason --- app/channel.go | 19 ++- app/channel_test.go | 69 ++++++++- store/sqlstore/compliance_store.go | 6 +- store/storetest/compliance_store.go | 298 +++++++++++++++++++++++++++++++++--- 4 files changed, 362 insertions(+), 30 deletions(-) diff --git a/app/channel.go b/app/channel.go index edece8c98..76147a508 100644 --- a/app/channel.go +++ b/app/channel.go @@ -225,6 +225,14 @@ func (a *App) createDirectChannel(userId string, otherUserId string) (*model.Cha } } else { channel := result.Data.(*model.Channel) + + if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(userId, channel.Id, model.GetMillis()); result.Err != nil { + l4g.Warn("Failed to update ChannelMemberHistory table %v", result.Err) + } + if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(otherUserId, channel.Id, model.GetMillis()); result.Err != nil { + l4g.Warn("Failed to update ChannelMemberHistory table %v", result.Err) + } + return channel, nil } } @@ -1426,7 +1434,16 @@ func (a *App) GetDirectChannel(userId1, userId2 string) (*model.Channel, *model. } a.InvalidateCacheForUser(userId1) a.InvalidateCacheForUser(userId2) - return result.Data.(*model.Channel), nil + + channel := result.Data.(*model.Channel) + if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(userId1, channel.Id, model.GetMillis()); result.Err != nil { + l4g.Warn("Failed to update ChannelMemberHistory table %v", result.Err) + } + if result := <-a.Srv.Store.ChannelMemberHistory().LogJoinEvent(userId2, channel.Id, model.GetMillis()); result.Err != nil { + l4g.Warn("Failed to update ChannelMemberHistory table %v", result.Err) + } + + return channel, nil } else if result.Err != nil { return nil, model.NewAppError("GetOrCreateDMChannel", "web.incoming_webhook.channel.app_error", nil, "err="+result.Err.Message, result.Err.StatusCode) } diff --git a/app/channel_test.go b/app/channel_test.go index e4a0e4320..69efaeca7 100644 --- a/app/channel_test.go +++ b/app/channel_test.go @@ -110,7 +110,7 @@ func TestMoveChannel(t *testing.T) { } } -func TestJoinDefaultChannelsTownSquare(t *testing.T) { +func TestJoinDefaultChannelsCreatesChannelMemberHistoryRecordTownSquare(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() @@ -136,7 +136,7 @@ func TestJoinDefaultChannelsTownSquare(t *testing.T) { assert.True(t, found) } -func TestJoinDefaultChannelsOffTopic(t *testing.T) { +func TestJoinDefaultChannelsCreatesChannelMemberHistoryRecordOffTopic(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() @@ -162,7 +162,7 @@ func TestJoinDefaultChannelsOffTopic(t *testing.T) { assert.True(t, found) } -func TestCreateChannelPublic(t *testing.T) { +func TestCreateChannelPublicCreatesChannelMemberHistoryRecord(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() @@ -176,7 +176,7 @@ func TestCreateChannelPublic(t *testing.T) { assert.Equal(t, publicChannel.Id, histories[0].ChannelId) } -func TestCreateChannelPrivate(t *testing.T) { +func TestCreateChannelPrivateCreatesChannelMemberHistoryRecord(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() @@ -205,7 +205,7 @@ func TestUpdateChannelPrivacy(t *testing.T) { } } -func TestCreateGroupChannel(t *testing.T) { +func TestCreateGroupChannelCreatesChannelMemberHistoryRecord(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() @@ -233,7 +233,62 @@ func TestCreateGroupChannel(t *testing.T) { } } -func TestAddUserToChannel(t *testing.T) { +func TestCreateDirectChannelCreatesChannelMemberHistoryRecord(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + user1 := th.CreateUser() + user2 := th.CreateUser() + + if channel, err := th.App.CreateDirectChannel(user1.Id, user2.Id); err != nil { + t.Fatal("Failed to create direct channel. Error: " + err.Message) + } else { + // there should be a ChannelMemberHistory record for both users + histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id)).([]*model.ChannelMemberHistoryResult) + assert.Len(t, histories, 2) + + historyId0 := histories[0].UserId + historyId1 := histories[1].UserId + switch historyId0 { + case user1.Id: + assert.Equal(t, user2.Id, historyId1) + case user2.Id: + assert.Equal(t, user1.Id, historyId1) + default: + t.Fatal("Unexpected user id " + historyId0 + " in ChannelMemberHistory table") + } + } +} + +func TestGetDirectChannelCreatesChannelMemberHistoryRecord(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + user1 := th.CreateUser() + user2 := th.CreateUser() + + // this function call implicitly creates a direct channel between the two users if one doesn't already exist + if channel, err := th.App.GetDirectChannel(user1.Id, user2.Id); err != nil { + t.Fatal("Failed to create direct channel. Error: " + err.Message) + } else { + // there should be a ChannelMemberHistory record for both users + histories := store.Must(th.App.Srv.Store.ChannelMemberHistory().GetUsersInChannelDuring(model.GetMillis()-100, model.GetMillis()+100, channel.Id)).([]*model.ChannelMemberHistoryResult) + assert.Len(t, histories, 2) + + historyId0 := histories[0].UserId + historyId1 := histories[1].UserId + switch historyId0 { + case user1.Id: + assert.Equal(t, user2.Id, historyId1) + case user2.Id: + assert.Equal(t, user1.Id, historyId1) + default: + t.Fatal("Unexpected user id " + historyId0 + " in ChannelMemberHistory table") + } + } +} + +func TestAddUserToChannelCreatesChannelMemberHistoryRecord(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() @@ -263,7 +318,7 @@ func TestAddUserToChannel(t *testing.T) { assert.Equal(t, groupUserIds, channelMemberHistoryUserIds) } -func TestRemoveUserFromChannel(t *testing.T) { +func TestRemoveUserFromChannelUpdatesChannelMemberHistoryRecord(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() diff --git a/store/sqlstore/compliance_store.go b/store/sqlstore/compliance_store.go index 03d92d5e1..121d80102 100644 --- a/store/sqlstore/compliance_store.go +++ b/store/sqlstore/compliance_store.go @@ -223,7 +223,11 @@ func (s SqlComplianceStore) MessageExport(after int64, limit int) store.StoreCha Posts.Type AS PostType, Posts.FileIds AS PostFileIds, Channels.Id AS ChannelId, - Channels.DisplayName AS ChannelDisplayName, + CASE + WHEN Channels.Type = 'D' THEN 'Direct Message' + WHEN Channels.Type = 'G' THEN 'Group Message' + ELSE Channels.DisplayName + END AS ChannelDisplayName, Users.Id AS UserId, Users.Email AS UserEmail, Users.Username diff --git a/store/storetest/compliance_store.go b/store/storetest/compliance_store.go index eb29bedc7..0a6b79cc5 100644 --- a/store/storetest/compliance_store.go +++ b/store/storetest/compliance_store.go @@ -16,7 +16,10 @@ func TestComplianceStore(t *testing.T, ss store.Store) { t.Run("", func(t *testing.T) { testComplianceStore(t, ss) }) t.Run("ComplianceExport", func(t *testing.T) { testComplianceExport(t, ss) }) t.Run("ComplianceExportDirectMessages", func(t *testing.T) { testComplianceExportDirectMessages(t, ss) }) - t.Run("MessageExport", func(t *testing.T) { testComplianceMessageExport(t, ss) }) + t.Run("MessageExportPublicChannel", func(t *testing.T) { testMessageExportPublicChannel(t, ss) }) + t.Run("MessageExportPrivateChannel", func(t *testing.T) { testMessageExportPrivateChannel(t, ss) }) + t.Run("MessageExportDirectMessageChannel", func(t *testing.T) { testMessageExportDirectMessageChannel(t, ss) }) + t.Run("MessageExportGroupMessageChannel", func(t *testing.T) { testMessageExportGroupMessageChannel(t, ss) }) } func testComplianceStore(t *testing.T, ss store.Store) { @@ -319,7 +322,7 @@ func testComplianceExportDirectMessages(t *testing.T, ss store.Store) { } } -func testComplianceMessageExport(t *testing.T, ss store.Store) { +func testMessageExportPublicChannel(t *testing.T, ss store.Store) { // get the starting number of message export entries startTime := model.GetMillis() var numMessageExports = 0 @@ -360,15 +363,14 @@ func testComplianceMessageExport(t *testing.T, ss store.Store) { UserId: user2.Id, }, -1)) - // need a public channel as well as a DM channel between the two users + // need a public channel channel := &model.Channel{ TeamId: team.Id, Name: model.NewId(), - DisplayName: "Channel2", + DisplayName: "Public Channel", Type: model.CHANNEL_OPEN, } channel = store.Must(ss.Channel().Save(channel, -1)).(*model.Channel) - directMessageChannel := store.Must(ss.Channel().CreateDirectChannel(user1.Id, user2.Id)).(*model.Channel) // user1 posts twice in the public channel post1 := &model.Post{ @@ -387,22 +389,114 @@ func testComplianceMessageExport(t *testing.T, ss store.Store) { } post2 = store.Must(ss.Post().Save(post2)).(*model.Post) - // user1 also sends a DM to user2 - post3 := &model.Post{ - ChannelId: directMessageChannel.Id, + // fetch the message exports for both posts that user1 sent + messageExportMap := map[string]model.MessageExport{} + if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil { + t.Fatal(r1.Err) + } else { + messages := r1.Data.([]*model.MessageExport) + assert.Equal(t, numMessageExports+2, len(messages)) + + for _, v := range messages { + messageExportMap[*v.PostId] = *v + } + } + + // post1 was made by user1 in channel1 and team1 + assert.Equal(t, post1.Id, *messageExportMap[post1.Id].PostId) + assert.Equal(t, post1.CreateAt, *messageExportMap[post1.Id].PostCreateAt) + assert.Equal(t, post1.Message, *messageExportMap[post1.Id].PostMessage) + assert.Equal(t, channel.Id, *messageExportMap[post1.Id].ChannelId) + assert.Equal(t, channel.DisplayName, *messageExportMap[post1.Id].ChannelDisplayName) + assert.Equal(t, user1.Id, *messageExportMap[post1.Id].UserId) + assert.Equal(t, user1.Email, *messageExportMap[post1.Id].UserEmail) + assert.Equal(t, user1.Username, *messageExportMap[post1.Id].Username) + + // post2 was made by user1 in channel1 and team1 + assert.Equal(t, post2.Id, *messageExportMap[post2.Id].PostId) + assert.Equal(t, post2.CreateAt, *messageExportMap[post2.Id].PostCreateAt) + assert.Equal(t, post2.Message, *messageExportMap[post2.Id].PostMessage) + assert.Equal(t, channel.Id, *messageExportMap[post2.Id].ChannelId) + assert.Equal(t, channel.DisplayName, *messageExportMap[post2.Id].ChannelDisplayName) + assert.Equal(t, user1.Id, *messageExportMap[post2.Id].UserId) + assert.Equal(t, user1.Email, *messageExportMap[post2.Id].UserEmail) + assert.Equal(t, user1.Username, *messageExportMap[post2.Id].Username) +} + +func testMessageExportPrivateChannel(t *testing.T, ss store.Store) { + // get the starting number of message export entries + startTime := model.GetMillis() + var numMessageExports = 0 + if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil { + t.Fatal(r1.Err) + } else { + messages := r1.Data.([]*model.MessageExport) + numMessageExports = len(messages) + } + + // need a team + team := &model.Team{ + DisplayName: "DisplayName", + Name: "zz" + model.NewId() + "b", + Email: model.NewId() + "@nowhere.com", + Type: model.TEAM_OPEN, + } + team = store.Must(ss.Team().Save(team)).(*model.Team) + + // and two users that are a part of that team + user1 := &model.User{ + Email: model.NewId(), + Username: model.NewId(), + } + user1 = store.Must(ss.User().Save(user1)).(*model.User) + store.Must(ss.Team().SaveMember(&model.TeamMember{ + TeamId: team.Id, + UserId: user1.Id, + }, -1)) + + user2 := &model.User{ + Email: model.NewId(), + Username: model.NewId(), + } + user2 = store.Must(ss.User().Save(user2)).(*model.User) + store.Must(ss.Team().SaveMember(&model.TeamMember{ + TeamId: team.Id, + UserId: user2.Id, + }, -1)) + + // need a private channel + channel := &model.Channel{ + TeamId: team.Id, + Name: model.NewId(), + DisplayName: "Private Channel", + Type: model.CHANNEL_PRIVATE, + } + channel = store.Must(ss.Channel().Save(channel, -1)).(*model.Channel) + + // user1 posts twice in the private channel + post1 := &model.Post{ + ChannelId: channel.Id, UserId: user1.Id, - CreateAt: startTime + 20, - Message: "zz" + model.NewId() + "c", + CreateAt: startTime, + Message: "zz" + model.NewId() + "a", + } + post1 = store.Must(ss.Post().Save(post1)).(*model.Post) + + post2 := &model.Post{ + ChannelId: channel.Id, + UserId: user1.Id, + CreateAt: startTime + 10, + Message: "zz" + model.NewId() + "b", } - post3 = store.Must(ss.Post().Save(post3)).(*model.Post) + post2 = store.Must(ss.Post().Save(post2)).(*model.Post) - // fetch the message exports for all three posts that user1 sent + // fetch the message exports for both posts that user1 sent messageExportMap := map[string]model.MessageExport{} if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil { t.Fatal(r1.Err) } else { messages := r1.Data.([]*model.MessageExport) - assert.Equal(t, numMessageExports+3, len(messages)) + assert.Equal(t, numMessageExports+2, len(messages)) for _, v := range messages { messageExportMap[*v.PostId] = *v @@ -428,13 +522,175 @@ func testComplianceMessageExport(t *testing.T, ss store.Store) { assert.Equal(t, user1.Id, *messageExportMap[post2.Id].UserId) assert.Equal(t, user1.Email, *messageExportMap[post2.Id].UserEmail) assert.Equal(t, user1.Username, *messageExportMap[post2.Id].Username) +} + +func testMessageExportDirectMessageChannel(t *testing.T, ss store.Store) { + // get the starting number of message export entries + startTime := model.GetMillis() + var numMessageExports = 0 + if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil { + t.Fatal(r1.Err) + } else { + messages := r1.Data.([]*model.MessageExport) + numMessageExports = len(messages) + } + + // need a team + team := &model.Team{ + DisplayName: "DisplayName", + Name: "zz" + model.NewId() + "b", + Email: model.NewId() + "@nowhere.com", + Type: model.TEAM_OPEN, + } + team = store.Must(ss.Team().Save(team)).(*model.Team) + + // and two users that are a part of that team + user1 := &model.User{ + Email: model.NewId(), + Username: model.NewId(), + } + user1 = store.Must(ss.User().Save(user1)).(*model.User) + store.Must(ss.Team().SaveMember(&model.TeamMember{ + TeamId: team.Id, + UserId: user1.Id, + }, -1)) + + user2 := &model.User{ + Email: model.NewId(), + Username: model.NewId(), + } + user2 = store.Must(ss.User().Save(user2)).(*model.User) + store.Must(ss.Team().SaveMember(&model.TeamMember{ + TeamId: team.Id, + UserId: user2.Id, + }, -1)) + + // as well as a DM channel between those users + directMessageChannel := store.Must(ss.Channel().CreateDirectChannel(user1.Id, user2.Id)).(*model.Channel) + + // user1 also sends a DM to user2 + post := &model.Post{ + ChannelId: directMessageChannel.Id, + UserId: user1.Id, + CreateAt: startTime + 20, + Message: "zz" + model.NewId() + "c", + } + post = store.Must(ss.Post().Save(post)).(*model.Post) + + // fetch the message export for the post that user1 sent + messageExportMap := map[string]model.MessageExport{} + if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil { + t.Fatal(r1.Err) + } else { + messages := r1.Data.([]*model.MessageExport) + assert.Equal(t, numMessageExports+1, len(messages)) + + for _, v := range messages { + messageExportMap[*v.PostId] = *v + } + } + + // post is a DM between user1 and user2 + // there is no channel display name for direct messages, so we sub in the string "Direct Message" instead + assert.Equal(t, post.Id, *messageExportMap[post.Id].PostId) + assert.Equal(t, post.CreateAt, *messageExportMap[post.Id].PostCreateAt) + assert.Equal(t, post.Message, *messageExportMap[post.Id].PostMessage) + assert.Equal(t, directMessageChannel.Id, *messageExportMap[post.Id].ChannelId) + assert.Equal(t, "Direct Message", *messageExportMap[post.Id].ChannelDisplayName) + assert.Equal(t, user1.Id, *messageExportMap[post.Id].UserId) + assert.Equal(t, user1.Email, *messageExportMap[post.Id].UserEmail) + assert.Equal(t, user1.Username, *messageExportMap[post.Id].Username) +} + +func testMessageExportGroupMessageChannel(t *testing.T, ss store.Store) { + // get the starting number of message export entries + startTime := model.GetMillis() + var numMessageExports = 0 + if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil { + t.Fatal(r1.Err) + } else { + messages := r1.Data.([]*model.MessageExport) + numMessageExports = len(messages) + } + + // need a team + team := &model.Team{ + DisplayName: "DisplayName", + Name: "zz" + model.NewId() + "b", + Email: model.NewId() + "@nowhere.com", + Type: model.TEAM_OPEN, + } + team = store.Must(ss.Team().Save(team)).(*model.Team) + + // and three users that are a part of that team + user1 := &model.User{ + Email: model.NewId(), + Username: model.NewId(), + } + user1 = store.Must(ss.User().Save(user1)).(*model.User) + store.Must(ss.Team().SaveMember(&model.TeamMember{ + TeamId: team.Id, + UserId: user1.Id, + }, -1)) + + user2 := &model.User{ + Email: model.NewId(), + Username: model.NewId(), + } + user2 = store.Must(ss.User().Save(user2)).(*model.User) + store.Must(ss.Team().SaveMember(&model.TeamMember{ + TeamId: team.Id, + UserId: user2.Id, + }, -1)) + + user3 := &model.User{ + Email: model.NewId(), + Username: model.NewId(), + } + user3 = store.Must(ss.User().Save(user3)).(*model.User) + store.Must(ss.Team().SaveMember(&model.TeamMember{ + TeamId: team.Id, + UserId: user3.Id, + }, -1)) + + // can't create a group channel directly, because importing app creates an import cycle, so we have to fake it + groupMessageChannel := &model.Channel{ + TeamId: team.Id, + Name: model.NewId(), + Type: model.CHANNEL_GROUP, + } + groupMessageChannel = store.Must(ss.Channel().Save(groupMessageChannel, -1)).(*model.Channel) + + // user1 posts in the GM + post := &model.Post{ + ChannelId: groupMessageChannel.Id, + UserId: user1.Id, + CreateAt: startTime + 20, + Message: "zz" + model.NewId() + "c", + } + post = store.Must(ss.Post().Save(post)).(*model.Post) + + // fetch the message export for the post that user1 sent + messageExportMap := map[string]model.MessageExport{} + if r1 := <-ss.Compliance().MessageExport(startTime-10, 10); r1.Err != nil { + t.Fatal(r1.Err) + } else { + messages := r1.Data.([]*model.MessageExport) + assert.Equal(t, numMessageExports+1, len(messages)) + + for _, v := range messages { + messageExportMap[*v.PostId] = *v + } + } - // post3 is a DM between user1 and user2 - assert.Equal(t, post3.Id, *messageExportMap[post3.Id].PostId) - assert.Equal(t, post3.CreateAt, *messageExportMap[post3.Id].PostCreateAt) - assert.Equal(t, post3.Message, *messageExportMap[post3.Id].PostMessage) - assert.Equal(t, directMessageChannel.Id, *messageExportMap[post3.Id].ChannelId) - assert.Equal(t, user1.Id, *messageExportMap[post3.Id].UserId) - assert.Equal(t, user1.Email, *messageExportMap[post3.Id].UserEmail) - assert.Equal(t, user1.Username, *messageExportMap[post3.Id].Username) + // post is a DM between user1 and user2 + // there is no channel display name for direct messages, so we sub in the string "Direct Message" instead + assert.Equal(t, post.Id, *messageExportMap[post.Id].PostId) + assert.Equal(t, post.CreateAt, *messageExportMap[post.Id].PostCreateAt) + assert.Equal(t, post.Message, *messageExportMap[post.Id].PostMessage) + assert.Equal(t, groupMessageChannel.Id, *messageExportMap[post.Id].ChannelId) + assert.Equal(t, "Group Message", *messageExportMap[post.Id].ChannelDisplayName) + assert.Equal(t, user1.Id, *messageExportMap[post.Id].UserId) + assert.Equal(t, user1.Email, *messageExportMap[post.Id].UserEmail) + assert.Equal(t, user1.Username, *messageExportMap[post.Id].Username) } -- cgit v1.2.3-1-g7c22