From 2ff29e375b722d7102dffbb6ca29eeeead1b4380 Mon Sep 17 00:00:00 2001 From: Maria A Nunez Date: Mon, 22 Jun 2026 13:52:05 -0400 Subject: [PATCH] [release-10.11] Tighten thread membership cleanup on team membership changes (#37081) * Tighten thread membership cleanup on team membership changes (#36764) * Tighten thread membership cleanup on team membership changes Ensure ThreadMembership rows are cleaned up when a user is removed from or leaves a team, add a defense-in-depth filter on the thread retrieval path so memberships referencing channels the user is no longer a member of are excluded, and add a one-time migration to clean up stale records on existing deployments. https://mattermost.atlassian.net/browse/MM-69008 * Centralize per-channel membership removal and tighten tests Extract the combined channel-member and thread-membership removal into a shared helper used by both removeUserFromChannel and LeaveTeam, so future code paths cannot revoke channel access without dropping the dependent thread state. The channel-leave event is now logged after the combined removal completes. Also drop verbose test header comments and rename a test to a behavior-focused name. * Backfill channel members in thread storetest setups The new ChannelMembers predicate on thread read queries filters out ThreadMembership rows whose user has no ChannelMembers row for the thread's channel. Several existing storetest setups bypassed the normal write path and inserted threads/memberships without channel members. Add the missing channel-member rows so the test data matches the real-world invariant. * Retrigger enterprise CI Pick up enterprise merge e6953d4 (master into MM-69008-thread-membership-team-leave) in the combined Enterprise CI/tests lane, which pins the enterprise SHA at mattermost-side dispatch time. Co-authored-by: Maria A Nunez --------- Co-authored-by: Mattermost Build Co-authored-by: Cursor Agent * Add MySQL migration for 000195_threadmemberships_cleanup_v2 Co-authored-by: Cursor * Apply pre-commit lint fixes Co-authored-by: Cursor * Retrigger CI Co-authored-by: Cursor --------- Co-authored-by: Mattermost Build Co-authored-by: Cursor Agent --- server/channels/api4/user_test.go | 57 ++++ server/channels/app/channel.go | 20 +- server/channels/app/team.go | 4 +- server/channels/app/team_test.go | 250 ++++++++++++++++++ server/channels/db/migrations/migrations.list | 4 + ...0195_threadmemberships_cleanup_v2.down.sql | 1 + ...000195_threadmemberships_cleanup_v2.up.sql | 7 + ...0195_threadmemberships_cleanup_v2.down.sql | 1 + ...000195_threadmemberships_cleanup_v2.up.sql | 13 + .../channels/store/sqlstore/thread_store.go | 29 +- .../channels/store/storetest/thread_store.go | 60 +++++ 11 files changed, 431 insertions(+), 15 deletions(-) create mode 100644 server/channels/db/migrations/mysql/000195_threadmemberships_cleanup_v2.down.sql create mode 100644 server/channels/db/migrations/mysql/000195_threadmemberships_cleanup_v2.up.sql create mode 100644 server/channels/db/migrations/postgres/000195_threadmemberships_cleanup_v2.down.sql create mode 100644 server/channels/db/migrations/postgres/000195_threadmemberships_cleanup_v2.up.sql diff --git a/server/channels/api4/user_test.go b/server/channels/api4/user_test.go index 80aaa75047..af258c85ad 100644 --- a/server/channels/api4/user_test.go +++ b/server/channels/api4/user_test.go @@ -7711,6 +7711,63 @@ func TestGetThreadsForUser(t *testing.T) { }) } +func TestGetThreadsForUser_AfterTeamRemovalAndReinvite(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.ThreadAutoFollow = true + *cfg.ServiceSettings.CollapsedThreads = model.CollapsedThreadsDefaultOn + }) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional)) + + admin := th.BasicUser + victim := th.BasicUser2 + + privateChannel := th.CreatePrivateChannel() + th.AddUserToChannel(victim, privateChannel) + + defer func() { + require.NoError(t, th.App.Srv().Store().Post().PermanentDeleteByUser(th.Context, admin.Id)) + require.NoError(t, th.App.Srv().Store().Post().PermanentDeleteByUser(th.Context, victim.Id)) + }() + + rootPost, _, err := th.Client.CreatePost(context.Background(), &model.Post{ + ChannelId: privateChannel.Id, + Message: "private team secret", + }) + require.NoError(t, err) + + victimClient := th.CreateClient() + th.LoginBasic2WithClient(victimClient) + + _, _, err = victimClient.CreatePost(context.Background(), &model.Post{ + ChannelId: privateChannel.Id, + RootId: rootPost.Id, + Message: "victim reply", + }) + require.NoError(t, err) + + uss, _, err := victimClient.GetUserThreads(context.Background(), victim.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{Extended: true}) + require.NoError(t, err) + require.Len(t, uss.Threads, 1, "sanity: victim should see their own thread before team removal") + + _, err = th.SystemAdminClient.RemoveTeamMember(context.Background(), th.BasicTeam.Id, victim.Id) + require.NoError(t, err) + + _, _, err = th.SystemAdminClient.AddTeamMember(context.Background(), th.BasicTeam.Id, victim.Id) + require.NoError(t, err) + + th.LoginBasic2WithClient(victimClient) + + uss, _, err = victimClient.GetUserThreads(context.Background(), victim.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{Extended: true}) + require.NoError(t, err) + for _, thr := range uss.Threads { + require.NotEqual(t, rootPost.Id, thr.PostId, "private-channel thread must not leak to re-invited user") + } + require.Len(t, uss.Threads, 0, "re-invited user must not receive any threads from private channels they no longer belong to") +} + func TestThreadSocketEvents(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic() diff --git a/server/channels/app/channel.go b/server/channels/app/channel.go index 4d72f81b8b..cacdd83ef1 100644 --- a/server/channels/app/channel.go +++ b/server/channels/app/channel.go @@ -2668,6 +2668,19 @@ func (a *App) postRemoveFromChannelMessage(c request.CTX, removerUserId string, return nil } +// removeChannelMembership strips a user's channel membership and the associated +// thread memberships. Keeping these together ensures channel access cannot be +// revoked without also dropping the thread state that depends on it. +func (a *App) removeChannelMembership(rctx request.CTX, userID, channelID, caller string) *model.AppError { + if err := a.Srv().Store().Channel().RemoveMember(rctx, channelID, userID); err != nil { + return model.NewAppError(caller, "app.channel.remove_member.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + if err := a.Srv().Store().Thread().DeleteMembershipsForChannel(userID, channelID); err != nil { + return model.NewAppError(caller, model.NoTranslation, nil, "failed to delete threadmemberships upon leaving channel", http.StatusInternalServerError).Wrap(err) + } + return nil +} + func (a *App) removeUserFromChannel(c request.CTX, userIDToRemove string, removerUserId string, channel *model.Channel) *model.AppError { user, nErr := a.Srv().Store().User().Get(context.Background(), userIDToRemove) if nErr != nil { @@ -2702,15 +2715,12 @@ func (a *App) removeUserFromChannel(c request.CTX, userIDToRemove string, remove return err } - if err := a.Srv().Store().Channel().RemoveMember(c, channel.Id, userIDToRemove); err != nil { - return model.NewAppError("removeUserFromChannel", "app.channel.remove_member.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + if appErr := a.removeChannelMembership(c, userIDToRemove, channel.Id, "removeUserFromChannel"); appErr != nil { + return appErr } if err := a.Srv().Store().ChannelMemberHistory().LogLeaveEvent(userIDToRemove, channel.Id, model.GetMillis()); err != nil { return model.NewAppError("removeUserFromChannel", "app.channel_member_history.log_leave_event.internal_error", nil, "", http.StatusInternalServerError).Wrap(err) } - if err := a.Srv().Store().Thread().DeleteMembershipsForChannel(userIDToRemove, channel.Id); err != nil { - return model.NewAppError("removeUserFromChannel", model.NoTranslation, nil, "failed to delete threadmemberships upon leaving channel", http.StatusInternalServerError).Wrap(err) - } if isGuest { currentMembers, err := a.GetChannelMembersForUser(c, channel.TeamId, userIDToRemove) diff --git a/server/channels/app/team.go b/server/channels/app/team.go index b1060e3ea9..e8bf0b259c 100644 --- a/server/channels/app/team.go +++ b/server/channels/app/team.go @@ -1237,8 +1237,8 @@ func (a *App) LeaveTeam(c request.CTX, team *model.Team, user *model.User, reque for _, channel := range channelList { if !channel.IsGroupOrDirect() { a.invalidateCacheForChannelMembers(channel.Id) - if nErr = a.Srv().Store().Channel().RemoveMember(c, channel.Id, user.Id); nErr != nil { - return model.NewAppError("LeaveTeam", "app.channel.remove_member.app_error", nil, "", http.StatusInternalServerError).Wrap(nErr) + if appErr := a.removeChannelMembership(c, user.Id, channel.Id, "LeaveTeam"); appErr != nil { + return appErr } } } diff --git a/server/channels/app/team_test.go b/server/channels/app/team_test.go index e47d691214..8a63242a61 100644 --- a/server/channels/app/team_test.go +++ b/server/channels/app/team_test.go @@ -1139,6 +1139,256 @@ func TestLeaveTeamPanic(t *testing.T) { }, "unexpected panic from LeaveTeam") } +func TestLeaveTeamCleansUpThreadMemberships(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.ThreadAutoFollow = true + *cfg.ServiceSettings.CollapsedThreads = model.CollapsedThreadsDefaultOn + }) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional)) + + admin := th.BasicUser + victim := th.BasicUser2 + + privateChannel := th.CreatePrivateChannel(th.Context, th.BasicTeam) + th.AddUserToChannel(victim, privateChannel) + + rootPost, _, appErr := th.App.CreatePost(th.Context, &model.Post{ + UserId: admin.Id, + ChannelId: privateChannel.Id, + Message: "private team secret", + }, privateChannel, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + defer func() { + require.NoError(t, th.App.Srv().Store().Post().PermanentDeleteByUser(th.Context, admin.Id)) + }() + + _, _, appErr = th.App.CreatePost(th.Context, &model.Post{ + UserId: victim.Id, + ChannelId: privateChannel.Id, + RootId: rootPost.Id, + Message: "victim reply", + }, privateChannel, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + defer func() { + require.NoError(t, th.App.Srv().Store().Post().PermanentDeleteByUser(th.Context, victim.Id)) + }() + + _, sErr := th.App.Srv().Store().Thread().GetMembershipForUser(victim.Id, rootPost.Id) + require.NoError(t, sErr, "victim should follow the thread after replying") + + appErr = th.App.LeaveTeam(th.Context, th.BasicTeam, victim, victim.Id) + require.Nil(t, appErr) + + _, gErr := th.App.Srv().Store().Thread().GetMembershipForUser(victim.Id, rootPost.Id) + var errNotFound *store.ErrNotFound + require.ErrorAs(t, gErr, &errNotFound, "thread membership must be deleted when user leaves the team") +} + +func TestLeaveTeamCleansUpThreadMembershipsAcrossChannels(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.ThreadAutoFollow = true + *cfg.ServiceSettings.CollapsedThreads = model.CollapsedThreadsDefaultOn + }) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional)) + + admin := th.BasicUser + victim := th.BasicUser2 + + privateA := th.CreatePrivateChannel(th.Context, th.BasicTeam) + privateB := th.CreatePrivateChannel(th.Context, th.BasicTeam) + openC := th.CreateChannel(th.Context, th.BasicTeam) + th.AddUserToChannel(victim, privateA) + th.AddUserToChannel(victim, privateB) + th.AddUserToChannel(victim, openC) + + rootIDs := make([]string, 0, 3) + for _, ch := range []*model.Channel{privateA, privateB, openC} { + root, _, appErr := th.App.CreatePost(th.Context, &model.Post{ + UserId: admin.Id, + ChannelId: ch.Id, + Message: "root in " + ch.Id, + }, ch, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + _, _, appErr = th.App.CreatePost(th.Context, &model.Post{ + UserId: victim.Id, + ChannelId: ch.Id, + RootId: root.Id, + Message: "reply", + }, ch, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + rootIDs = append(rootIDs, root.Id) + } + defer func() { + require.NoError(t, th.App.Srv().Store().Post().PermanentDeleteByUser(th.Context, admin.Id)) + require.NoError(t, th.App.Srv().Store().Post().PermanentDeleteByUser(th.Context, victim.Id)) + }() + + for _, rid := range rootIDs { + _, sErr := th.App.Srv().Store().Thread().GetMembershipForUser(victim.Id, rid) + require.NoError(t, sErr, "sanity: victim should follow each thread") + } + + appErr := th.App.LeaveTeam(th.Context, th.BasicTeam, victim, victim.Id) + require.Nil(t, appErr) + + var errNotFound *store.ErrNotFound + for _, rid := range rootIDs { + _, gErr := th.App.Srv().Store().Thread().GetMembershipForUser(victim.Id, rid) + require.ErrorAs(t, gErr, &errNotFound, "thread membership for %s must be deleted on team leave", rid) + } +} + +func TestLeaveTeamPreservesDMThreadMemberships(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.ThreadAutoFollow = true + *cfg.ServiceSettings.CollapsedThreads = model.CollapsedThreadsDefaultOn + }) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional)) + + admin := th.BasicUser + victim := th.BasicUser2 + + dmChannel, appErr := th.App.GetOrCreateDirectChannel(th.Context, admin.Id, victim.Id) + require.Nil(t, appErr) + + dmRoot, _, appErr := th.App.CreatePost(th.Context, &model.Post{ + UserId: admin.Id, + ChannelId: dmChannel.Id, + Message: "dm root", + }, dmChannel, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + _, _, appErr = th.App.CreatePost(th.Context, &model.Post{ + UserId: victim.Id, + ChannelId: dmChannel.Id, + RootId: dmRoot.Id, + Message: "dm reply", + }, dmChannel, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + defer func() { + require.NoError(t, th.App.Srv().Store().Post().PermanentDeleteByUser(th.Context, admin.Id)) + require.NoError(t, th.App.Srv().Store().Post().PermanentDeleteByUser(th.Context, victim.Id)) + }() + + _, sErr := th.App.Srv().Store().Thread().GetMembershipForUser(victim.Id, dmRoot.Id) + require.NoError(t, sErr, "sanity: victim should follow the DM thread") + + appErr = th.App.LeaveTeam(th.Context, th.BasicTeam, victim, victim.Id) + require.Nil(t, appErr) + + _, gErr := th.App.Srv().Store().Thread().GetMembershipForUser(victim.Id, dmRoot.Id) + require.NoError(t, gErr, "DM thread membership must survive leaving an unrelated team") +} + +func TestGetThreadsForUser_ReadPathRejectsOrphanThreadMembership(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.ThreadAutoFollow = true + *cfg.ServiceSettings.CollapsedThreads = model.CollapsedThreadsDefaultOn + }) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional)) + + admin := th.BasicUser + victim := th.BasicUser2 + + privateChannel := th.CreatePrivateChannel(th.Context, th.BasicTeam) + th.AddUserToChannel(victim, privateChannel) + + rootPost, _, appErr := th.App.CreatePost(th.Context, &model.Post{ + UserId: admin.Id, + ChannelId: privateChannel.Id, + Message: "private team secret", + }, privateChannel, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + defer func() { + require.NoError(t, th.App.Srv().Store().Post().PermanentDeleteByUser(th.Context, admin.Id)) + require.NoError(t, th.App.Srv().Store().Post().PermanentDeleteByUser(th.Context, victim.Id)) + }() + + _, _, appErr = th.App.CreatePost(th.Context, &model.Post{ + UserId: victim.Id, + ChannelId: privateChannel.Id, + RootId: rootPost.Id, + Message: "victim reply", + }, privateChannel, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + + _, sErr := th.App.Srv().Store().Thread().GetMembershipForUser(victim.Id, rootPost.Id) + require.NoError(t, sErr, "sanity: victim should follow the thread after replying") + + require.NoError(t, th.App.Srv().Store().Channel().RemoveMember(th.Context, privateChannel.Id, victim.Id)) + + _, sErr2 := th.App.Srv().Store().Thread().GetMembershipForUser(victim.Id, rootPost.Id) + require.NoError(t, sErr2, "sanity: synthetic orphan ThreadMembership must remain") + + threads, gErr := th.App.Srv().Store().Thread().GetThreadsForUser(victim.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{}) + require.NoError(t, gErr) + for _, thr := range threads { + require.NotEqual(t, rootPost.Id, thr.PostId, "read path must not surface threads from channels the user no longer belongs to") + } + require.Empty(t, threads, "GetThreadsForUser must filter out orphan ThreadMembership rows") + + totalThreads, gErr := th.App.Srv().Store().Thread().GetTotalThreads(victim.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{}) + require.NoError(t, gErr) + require.Zero(t, totalThreads, "GetTotalThreads must not count orphan ThreadMembership rows") + + totalUnread, gErr := th.App.Srv().Store().Thread().GetTotalUnreadThreads(victim.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{}) + require.NoError(t, gErr) + require.Zero(t, totalUnread, "GetTotalUnreadThreads must not count orphan ThreadMembership rows") +} + +func TestPermanentDeleteChannelRemovesThreadMemberships(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.ThreadAutoFollow = true + *cfg.ServiceSettings.CollapsedThreads = model.CollapsedThreadsDefaultOn + }) + th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional)) + + admin := th.BasicUser + victim := th.BasicUser2 + + privateChannel := th.CreatePrivateChannel(th.Context, th.BasicTeam) + th.AddUserToChannel(victim, privateChannel) + + rootPost, _, appErr := th.App.CreatePost(th.Context, &model.Post{ + UserId: admin.Id, + ChannelId: privateChannel.Id, + Message: "doomed root", + }, privateChannel, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + + _, _, appErr = th.App.CreatePost(th.Context, &model.Post{ + UserId: victim.Id, + ChannelId: privateChannel.Id, + RootId: rootPost.Id, + Message: "doomed reply", + }, privateChannel, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + + _, sErr := th.App.Srv().Store().Thread().GetMembershipForUser(victim.Id, rootPost.Id) + require.NoError(t, sErr, "victim should follow the thread after replying") + + appErr = th.App.PermanentDeleteChannel(th.Context, privateChannel) + require.Nil(t, appErr) + + _, gErr := th.App.Srv().Store().Thread().GetMembershipForUser(victim.Id, rootPost.Id) + var errNotFound *store.ErrNotFound + require.ErrorAs(t, gErr, &errNotFound, "thread membership must be deleted with the channel") +} + func TestAppUpdateTeamScheme(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic() diff --git a/server/channels/db/migrations/migrations.list b/server/channels/db/migrations/migrations.list index 2a85e8eace..c24ac1f980 100644 --- a/server/channels/db/migrations/migrations.list +++ b/server/channels/db/migrations/migrations.list @@ -285,6 +285,8 @@ channels/db/migrations/mysql/000143_backfill_roles_schemeid.down.sql channels/db/migrations/mysql/000143_backfill_roles_schemeid.up.sql channels/db/migrations/mysql/000144_add_roles_schemeid_index.down.sql channels/db/migrations/mysql/000144_add_roles_schemeid_index.up.sql +channels/db/migrations/mysql/000195_threadmemberships_cleanup_v2.down.sql +channels/db/migrations/mysql/000195_threadmemberships_cleanup_v2.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 @@ -571,3 +573,5 @@ channels/db/migrations/postgres/000143_backfill_roles_schemeid.down.sql channels/db/migrations/postgres/000143_backfill_roles_schemeid.up.sql channels/db/migrations/postgres/000144_add_roles_schemeid_index.down.sql channels/db/migrations/postgres/000144_add_roles_schemeid_index.up.sql +channels/db/migrations/postgres/000195_threadmemberships_cleanup_v2.down.sql +channels/db/migrations/postgres/000195_threadmemberships_cleanup_v2.up.sql diff --git a/server/channels/db/migrations/mysql/000195_threadmemberships_cleanup_v2.down.sql b/server/channels/db/migrations/mysql/000195_threadmemberships_cleanup_v2.down.sql new file mode 100644 index 0000000000..44724bc5de --- /dev/null +++ b/server/channels/db/migrations/mysql/000195_threadmemberships_cleanup_v2.down.sql @@ -0,0 +1 @@ +-- Skipping it because the forward migration is destructive diff --git a/server/channels/db/migrations/mysql/000195_threadmemberships_cleanup_v2.up.sql b/server/channels/db/migrations/mysql/000195_threadmemberships_cleanup_v2.up.sql new file mode 100644 index 0000000000..14eedbe516 --- /dev/null +++ b/server/channels/db/migrations/mysql/000195_threadmemberships_cleanup_v2.up.sql @@ -0,0 +1,7 @@ +-- Drop ThreadMembership rows whose user is no longer a member of the thread's channel. +DELETE tm +FROM ThreadMemberships AS tm +JOIN Threads ON Threads.PostId = tm.PostId +LEFT JOIN ChannelMembers ON ChannelMembers.UserId = tm.UserId + AND Threads.ChannelId = ChannelMembers.ChannelId +WHERE ChannelMembers.ChannelId IS NULL; diff --git a/server/channels/db/migrations/postgres/000195_threadmemberships_cleanup_v2.down.sql b/server/channels/db/migrations/postgres/000195_threadmemberships_cleanup_v2.down.sql new file mode 100644 index 0000000000..44724bc5de --- /dev/null +++ b/server/channels/db/migrations/postgres/000195_threadmemberships_cleanup_v2.down.sql @@ -0,0 +1 @@ +-- Skipping it because the forward migration is destructive diff --git a/server/channels/db/migrations/postgres/000195_threadmemberships_cleanup_v2.up.sql b/server/channels/db/migrations/postgres/000195_threadmemberships_cleanup_v2.up.sql new file mode 100644 index 0000000000..7d30131137 --- /dev/null +++ b/server/channels/db/migrations/postgres/000195_threadmemberships_cleanup_v2.up.sql @@ -0,0 +1,13 @@ +-- Drop ThreadMembership rows whose user is no longer a member of the thread's channel. +DELETE FROM threadmemberships WHERE (postid, userid) IN ( + SELECT + threadmemberships.postid, + threadmemberships.userid + FROM + threadmemberships + JOIN threads ON threads.postid = threadmemberships.postid + LEFT JOIN channelmembers ON channelmembers.userid = threadmemberships.userid + AND threads.channelid = channelmembers.channelid + WHERE + channelmembers.channelid IS NULL +); diff --git a/server/channels/store/sqlstore/thread_store.go b/server/channels/store/sqlstore/thread_store.go index de3c421450..b1c0af7e24 100644 --- a/server/channels/store/sqlstore/thread_store.go +++ b/server/channels/store/sqlstore/thread_store.go @@ -70,6 +70,17 @@ type SqlThreadStore struct { func (s *SqlThreadStore) ClearCaches() { } +// channelMembershipPredicate filters out ThreadMemberships whose user is no +// longer a member of the thread's channel. DM/GM threads have an empty +// ThreadTeamId and are exempt because their access is intrinsic to the +// channel members. +func channelMembershipPredicate() sq.Sqlizer { + return sq.Or{ + sq.Eq{"Threads.ThreadTeamId": ""}, + sq.Expr("EXISTS (SELECT 1 FROM ChannelMembers WHERE ChannelMembers.ChannelId = Threads.ChannelId AND ChannelMembers.UserId = ThreadMemberships.UserId)"), + } +} + func newSqlThreadStore(sqlStore *SqlStore) store.ThreadStore { s := SqlThreadStore{ SqlStore: sqlStore, @@ -131,7 +142,8 @@ func (s *SqlThreadStore) getTotalThreadsQuery(userId, teamId string, opts model. Where(sq.Eq{ "ThreadMemberships.UserId": userId, "ThreadMemberships.Following": true, - }) + }). + Where(channelMembershipPredicate()) if teamId != "" { if opts.ExcludeDirect { @@ -198,7 +210,8 @@ func (s *SqlThreadStore) GetTotalUnreadMentions(userId, teamId string, opts mode Where(sq.Eq{ "ThreadMemberships.UserId": userId, "ThreadMemberships.Following": true, - }) + }). + Where(channelMembershipPredicate()) if teamId != "" { if opts.ExcludeDirect { @@ -234,15 +247,13 @@ func (s *SqlThreadStore) GetTotalUnreadUrgentMentions(userId, teamId string, opt Select("COALESCE(SUM(ThreadMemberships.UnreadMentions),0)"). From("ThreadMemberships"). Join("PostsPriority ON PostsPriority.PostId = ThreadMemberships.PostId"). + Join("Threads ON Threads.PostId = ThreadMemberships.PostId"). Where(sq.Eq{ "ThreadMemberships.UserId": userId, "ThreadMemberships.Following": true, "PostsPriority.Priority": model.PostPriorityUrgent, - }) - - if teamId != "" || !opts.Deleted { - query = query.Join("Threads ON Threads.PostId = ThreadMemberships.PostId") - } + }). + Where(channelMembershipPredicate()) if teamId != "" { if opts.ExcludeDirect { @@ -298,7 +309,8 @@ func (s *SqlThreadStore) GetThreadsForUser(userId, teamId string, opts model.Get query = query. Where(sq.Eq{"ThreadMemberships.UserId": userId}). - Where(sq.Eq{"ThreadMemberships.Following": true}) + Where(sq.Eq{"ThreadMemberships.Following": true}). + Where(channelMembershipPredicate()) if opts.IncludeIsUrgent { urgencyCase := sq. @@ -405,6 +417,7 @@ func (s *SqlThreadStore) GetTeamsUnreadForUser(userID string, teamIDs []string, sq.Eq{"ThreadMemberships.Following": true}, sq.Eq{"Threads.ThreadTeamId": teamIDs}, sq.Eq{"COALESCE(Threads.ThreadDeleteAt, 0)": 0}, + channelMembershipPredicate(), } var eg errgroup.Group diff --git a/server/channels/store/storetest/thread_store.go b/server/channels/store/storetest/thread_store.go index 3a682f04d6..fe29f94d81 100644 --- a/server/channels/store/storetest/thread_store.go +++ b/server/channels/store/storetest/thread_store.go @@ -717,6 +717,12 @@ func testGetTeamsUnreadForUser(t *testing.T, rctx request.CTX, ss store.Store) { Type: model.ChannelTypeOpen, }, -1) require.NoError(t, err) + _, err = ss.Channel().SaveMember(rctx, &model.ChannelMember{ + ChannelId: channel1.Id, + UserId: userID, + NotifyProps: model.GetDefaultChannelNotifyProps(), + }) + require.NoError(t, err) post, err := ss.Post().Save(rctx, &model.Post{ ChannelId: channel1.Id, UserId: userID, @@ -759,6 +765,12 @@ func testGetTeamsUnreadForUser(t *testing.T, rctx request.CTX, ss store.Store) { Type: model.ChannelTypeOpen, }, -1) require.NoError(t, err) + _, err = ss.Channel().SaveMember(rctx, &model.ChannelMember{ + ChannelId: channel2.Id, + UserId: userID, + NotifyProps: model.GetDefaultChannelNotifyProps(), + }) + require.NoError(t, err) post2, err := ss.Post().Save(rctx, &model.Post{ ChannelId: channel2.Id, @@ -869,6 +881,12 @@ func testVarious(t *testing.T, rctx request.CTX, ss store.Store) { Type: model.ChannelTypeOpen, }, -1) require.NoError(t, err) + _, err = ss.Channel().SaveMember(rctx, &model.ChannelMember{ + ChannelId: team1channel1.Id, + UserId: user1ID, + NotifyProps: model.GetDefaultChannelNotifyProps(), + }) + require.NoError(t, err) team2channel1, err := ss.Channel().Save(rctx, &model.Channel{ TeamId: team2.Id, @@ -877,6 +895,12 @@ func testVarious(t *testing.T, rctx request.CTX, ss store.Store) { Type: model.ChannelTypeOpen, }, -1) require.NoError(t, err) + _, err = ss.Channel().SaveMember(rctx, &model.ChannelMember{ + ChannelId: team2channel1.Id, + UserId: user1ID, + NotifyProps: model.GetDefaultChannelNotifyProps(), + }) + require.NoError(t, err) dm1, err := ss.Channel().CreateDirectChannel(rctx, &model.User{Id: user1ID}, &model.User{Id: user2ID}) require.NoError(t, err) @@ -1345,6 +1369,17 @@ func testMarkAllAsReadByChannels(t *testing.T, rctx request.CTX, ss store.Store) }, -1) require.NoError(t, err) + for _, ch := range []*model.Channel{channel1, channel2} { + for _, uid := range []string{userAID, userBID} { + _, err = ss.Channel().SaveMember(rctx, &model.ChannelMember{ + ChannelId: ch.Id, + UserId: uid, + NotifyProps: model.GetDefaultChannelNotifyProps(), + }) + require.NoError(t, err) + } + } + createThreadMembership := func(userID, postID string) { t.Helper() opts := store.ThreadMembershipOpts{ @@ -1535,6 +1570,17 @@ func testMarkAllAsReadByTeam(t *testing.T, rctx request.CTX, ss store.Store) { }, -1) require.NoError(t, err) + for _, ch := range []*model.Channel{team1channel1, team1channel2, team2channel1, team2channel2} { + for _, uid := range []string{userAID, userBID} { + _, err = ss.Channel().SaveMember(rctx, &model.ChannelMember{ + ChannelId: ch.Id, + UserId: uid, + NotifyProps: model.GetDefaultChannelNotifyProps(), + }) + require.NoError(t, err) + } + } + team1channel1post1, err := ss.Post().Save(rctx, &model.Post{ ChannelId: team1channel1.Id, UserId: postingUserId, @@ -2110,6 +2156,13 @@ func testUpdateTeamIdForChannelThreads(t *testing.T, rctx request.CTX, ss store. }) require.NoError(t, err) + _, err = ss.Channel().SaveMember(rctx, &model.ChannelMember{ + ChannelId: channel1.Id, + UserId: userA.Id, + NotifyProps: model.GetDefaultChannelNotifyProps(), + }) + require.NoError(t, err) + _, clean := createThreadMembership(userA.Id, rootPost1.Id, true) defer clean() @@ -2134,6 +2187,13 @@ func testUpdateTeamIdForChannelThreads(t *testing.T, rctx request.CTX, ss store. }) require.NoError(t, err) + _, err = ss.Channel().SaveMember(rctx, &model.ChannelMember{ + ChannelId: channel1.Id, + UserId: userA.Id, + NotifyProps: model.GetDefaultChannelNotifyProps(), + }) + require.NoError(t, err) + newTeamID := model.NewId() _, clean := createThreadMembership(userA.Id, rootPost1.Id, true)