diff --git a/server/command_test.go b/server/command_test.go index 685f7507..1425c602 100644 --- a/server/command_test.go +++ b/server/command_test.go @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/require" "github.com/mattermost/mattermost-plugin-jira/server/enterprise" + "github.com/mattermost/mattermost-plugin-jira/server/utils/kvstore" "github.com/mattermost/mattermost-plugin-jira/server/utils/types" ) @@ -48,7 +49,7 @@ var _ UserStore = (*mockUserStoreKV)(nil) func (store mockUserStoreKV) LoadConnection(instanceID, mattermostUserID types.ID) (*Connection, error) { connection, ok := store.connections[mattermostUserID] if !ok { - return nil, errors.Errorf("TESTING connection %q %q not found", instanceID, mattermostUserID) + return nil, errors.Wrapf(kvstore.ErrNotFound, "TESTING connection %q %q", instanceID, mattermostUserID) } return connection, nil } @@ -99,6 +100,14 @@ func getMockUserStoreKV() mockUserStoreKV { } } +func mockUserStoreKVWithConnected(connectedUserIDs ...types.ID) mockUserStoreKV { + store := getMockUserStoreKV() + for _, id := range connectedUserIDs { + store.connections[id] = &Connection{User: jira.User{AccountID: "test-AccountID"}} + } + return store +} + type mockInstanceStoreKV struct { mockInstanceStore kv *sync.Map diff --git a/server/subscribe.go b/server/subscribe.go index 0d485945..95b942bd 100755 --- a/server/subscribe.go +++ b/server/subscribe.go @@ -20,6 +20,7 @@ import ( "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost-plugin-jira/server/utils" + "github.com/mattermost/mattermost-plugin-jira/server/utils/kvstore" "github.com/mattermost/mattermost-plugin-jira/server/utils/types" ) @@ -40,6 +41,7 @@ const ( CommentVisibility = "commentVisibility" TeamFilter = "teamField" CommentVisibilityGroupType = "group" + maxDMGMChannelMembers = 200 ) type FieldFilter struct { @@ -444,13 +446,66 @@ func (p *Plugin) removeChannelSubscription(instanceID types.ID, subscriptionID s }) } +func isDirectOrGroupChannel(channel *model.Channel) bool { + return channel.Type == model.ChannelTypeDirect || channel.Type == model.ChannelTypeGroup +} + +// channelHasConnectedMember reports whether any non-bot member of channel is still +// connected to instanceID. Channel types other than DM/GM are always allowed, since +// they are not tied to any single user's connection. +func (p *Plugin) channelHasConnectedMember(instanceID types.ID, channel *model.Channel) (bool, error) { + if !isDirectOrGroupChannel(channel) { + return true, nil + } + + botUserID := p.getConfig().botUserID + members, err := p.client.Channel.ListMembers(channel.Id, 0, maxDMGMChannelMembers) + if err != nil { + return false, err + } + + for _, member := range members { + if member.UserId == botUserID { + continue + } + + connection, err := p.userStore.LoadConnection(instanceID, types.ID(member.UserId)) + if err != nil { + if errors.Cause(err) != kvstore.ErrNotFound { + return false, err + } + continue + } + + // A missing connection can also read back as an empty one rather than an error. + if connection.JiraAccountID() != "" { + return true, nil + } + } + + return false, nil +} + +// subscriptionIDsForChannel returns the IDs of subscriptions targeting channelID. +// ByID is scanned directly because the IDByChannelID index can drift from it, e.g. +// for data written before the index existed. +func subscriptionIDsForChannel(subs *ChannelSubscriptions, channelID string) []string { + var subIDs []string + for id, sub := range subs.ByID { + if sub.ChannelID == channelID { + subIDs = append(subIDs, id) + } + } + return subIDs +} + func (p *Plugin) removeSubscriptionsForChannel(instanceID types.ID, channelID string) error { subs, err := p.getSubscriptions(instanceID) if err != nil { return err } - if subs.Channel.IDByChannelID[channelID].Len() == 0 { + if len(subscriptionIDsForChannel(subs.Channel, channelID)) == 0 { return nil } @@ -461,8 +516,7 @@ func (p *Plugin) removeSubscriptionsForChannel(instanceID types.ID, channelID st return nil, err } - subIDs := subs.Channel.IDByChannelID[channelID] - for _, subID := range subIDs.Elems() { + for _, subID := range subscriptionIDsForChannel(subs.Channel, channelID) { if sub, ok := subs.Channel.ByID[subID]; ok { subs.Channel.remove(&sub) } @@ -477,22 +531,62 @@ func (p *Plugin) removeSubscriptionsForChannel(instanceID types.ID, channelID st }) } +// cleanupDMSubscriptionsOnDisconnect removes channel subscriptions targeting any DM or +// GM the disconnecting user belongs to, once no member of that channel remains connected +// to instanceID. Must be called after the user's connection has been deleted. func (p *Plugin) cleanupDMSubscriptionsOnDisconnect(instanceID types.ID, mattermostUserID string) { - conf := p.getConfig() - dmChannel, err := p.client.Channel.GetDirect(mattermostUserID, conf.botUserID) + subs, err := p.getSubscriptions(instanceID) if err != nil { - p.client.Log.Warn("Failed to get DM channel for subscription cleanup on disconnect", + p.client.Log.Warn("Failed to load subscriptions for DM/GM cleanup on disconnect", "mattermostUserID", mattermostUserID, "instanceID", string(instanceID), "error", err.Error()) return } - if err := p.removeSubscriptionsForChannel(instanceID, dmChannel.Id); err != nil { - p.client.Log.Warn("Failed to clean up DM subscriptions on disconnect", - "mattermostUserID", mattermostUserID, - "instanceID", string(instanceID), - "error", err.Error()) + channelIDs := map[string]bool{} + for _, sub := range subs.Channel.ByID { + channelIDs[sub.ChannelID] = true + } + + for channelID := range channelIDs { + channel, err := p.client.Channel.Get(channelID) + if err != nil { + p.client.Log.Warn("Failed to get channel for DM/GM subscription cleanup on disconnect", + "channelID", channelID, + "instanceID", string(instanceID), + "error", err.Error()) + continue + } + + if !isDirectOrGroupChannel(channel) { + continue + } + + if _, err := p.client.Channel.GetMember(channelID, mattermostUserID); err != nil { + // The disconnecting user isn't a member of this DM/GM. + continue + } + + hasConnectedMember, err := p.channelHasConnectedMember(instanceID, channel) + if err != nil { + p.client.Log.Warn("Failed to check for connected members during DM/GM subscription cleanup", + "channelID", channelID, + "instanceID", string(instanceID), + "error", err.Error()) + continue + } + if hasConnectedMember { + continue + } + + if err := p.removeSubscriptionsForChannel(instanceID, channelID); err != nil { + p.client.Log.Warn("Failed to clean up DM/GM subscriptions on disconnect", + "mattermostUserID", mattermostUserID, + "channelID", channelID, + "instanceID", string(instanceID), + "error", err.Error()) + } } } diff --git a/server/subscribe_test.go b/server/subscribe_test.go index 563045d9..6d759491 100644 --- a/server/subscribe_test.go +++ b/server/subscribe_test.go @@ -18,6 +18,8 @@ import ( "github.com/mattermost/mattermost/server/public/plugin/plugintest/mock" "github.com/mattermost/mattermost/server/public/pluginapi" "github.com/stretchr/testify/assert" + + "github.com/mattermost/mattermost-plugin-jira/server/utils/types" ) func TestValidateSubscription(t *testing.T) { @@ -1679,6 +1681,135 @@ func TestGetChannelsSubscribed(t *testing.T) { } } +func TestChannelHasConnectedMember(t *testing.T) { + botUserID := "botuser___________________" + connectedUserID := types.ID("connecteduser______________") + disconnectedUserID := types.ID("disconnecteduser___________") + emptyConnectionUserID := types.ID("emptyconnuser______________") + + for name, tc := range map[string]struct { + channel *model.Channel + members model.ChannelMembers + expectHasConnected bool + }{ + "non-DM/GM channels are always allowed without checking membership": { + channel: &model.Channel{Id: "openchannel", Type: model.ChannelTypeOpen}, + expectHasConnected: true, + }, + "DM with a connected member": { + channel: &model.Channel{Id: "dmchannel", Type: model.ChannelTypeDirect}, + members: model.ChannelMembers{ + {UserId: botUserID}, + {UserId: connectedUserID.String()}, + }, + expectHasConnected: true, + }, + "DM with no connected member": { + channel: &model.Channel{Id: "dmchannel", Type: model.ChannelTypeDirect}, + members: model.ChannelMembers{ + {UserId: botUserID}, + {UserId: disconnectedUserID.String()}, + }, + expectHasConnected: false, + }, + "GM with one connected member out of three": { + channel: &model.Channel{Id: "gmchannel", Type: model.ChannelTypeGroup}, + members: model.ChannelMembers{ + {UserId: botUserID}, + {UserId: disconnectedUserID.String()}, + {UserId: connectedUserID.String()}, + }, + expectHasConnected: true, + }, + "GM with no connected members": { + channel: &model.Channel{Id: "gmchannel", Type: model.ChannelTypeGroup}, + members: model.ChannelMembers{ + {UserId: botUserID}, + {UserId: disconnectedUserID.String()}, + }, + expectHasConnected: false, + }, + // The KV store reads a missing connection back as an empty one rather than an error. + "DM whose member has an empty stored connection": { + channel: &model.Channel{Id: "dmchannel", Type: model.ChannelTypeDirect}, + members: model.ChannelMembers{ + {UserId: botUserID}, + {UserId: emptyConnectionUserID.String()}, + }, + expectHasConnected: false, + }, + } { + t.Run(name, func(t *testing.T) { + api := &plugintest.API{} + p := Plugin{} + + p.updateConfig(func(conf *config) { + conf.Secret = someSecret + conf.botUserID = botUserID + }) + p.SetAPI(api) + p.client = pluginapi.NewClient(api, p.Driver) + + userStore := mockUserStoreKVWithConnected(connectedUserID) + userStore.connections[emptyConnectionUserID] = &Connection{} + p.userStore = userStore + + if isDirectOrGroupChannel(tc.channel) { + api.On("GetChannelMembers", tc.channel.Id, 0, maxDMGMChannelMembers).Return(tc.members, nil) + } + + hasConnected, err := p.channelHasConnectedMember(testInstance1.GetID(), tc.channel) + require.NoError(t, err) + assert.Equal(t, tc.expectHasConnected, hasConnected) + }) + } +} + +func TestRemoveSubscriptionsForChannelIndexDrift(t *testing.T) { + dmChannelID := "dmchannelaaaaaaaaaaaaaaaa" + + api := &plugintest.API{} + p := Plugin{} + + p.updateConfig(func(conf *config) { + conf.Secret = someSecret + }) + p.SetAPI(api) + p.client = pluginapi.NewClient(api, p.Driver) + p.instanceStore = p.getMockInstanceStoreKV(1) + + // Simulate index drift: the subscription is present in ByID but missing + // from IDByChannelID, which removeSubscriptionsForChannel must not rely on. + existing := NewSubscriptions() + existing.Channel.ByID["sub1______________________"] = ChannelSubscription{ + ID: "sub1______________________", + ChannelID: dmChannelID, + InstanceID: testInstance1.GetID(), + Filters: SubscriptionFilters{ + Events: NewStringSet("jira:issue_created"), + Projects: NewStringSet("myproject"), + IssueTypes: NewStringSet("10001"), + }, + } + require.Equal(t, 0, existing.Channel.IDByChannelID[dmChannelID].Len()) + + existingBytes, err := json.Marshal(existing) + require.NoError(t, err) + + api.On("KVGet", testSubKey).Return(existingBytes, nil) + api.On("KVSetWithOptions", testSubKey, mock.MatchedBy(func(data []byte) bool { + var savedSubs Subscriptions + if unmarshalErr := json.Unmarshal(data, &savedSubs); unmarshalErr != nil { + return false + } + return len(savedSubs.Channel.ByID) == 0 + }), mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil) + + err = p.removeSubscriptionsForChannel(testInstance1.GetID(), dmChannelID) + assert.NoError(t, err) + api.AssertExpectations(t) +} + func TestRemoveSubscriptionsForChannel(t *testing.T) { dmChannelID := "dmchannelaaaaaaaaaaaaaaaa" otherChannelID := "otherchannelbbbbbbbbbbbbbb" @@ -1808,12 +1939,26 @@ func TestRemoveSubscriptionsForChannel(t *testing.T) { func TestCleanupDMSubscriptionsOnDisconnect(t *testing.T) { botUserID := "botuser___________________" mattermostUserID := "mmuser____________________" + otherMemberID := "othermember________________" dmChannelID := "dmchannelaaaaaaaaaaaaaaaa" + gmChannelID := "gmchannelbbbbbbbbbbbbbbbb" otherChannelID := "otherchannelbbbbbbbbbbbbbb" - t.Run("removes DM subscriptions on disconnect", func(t *testing.T) { + newSub := func(id, channelID string) ChannelSubscription { + return ChannelSubscription{ + ID: id, + ChannelID: channelID, + Filters: SubscriptionFilters{ + Events: NewStringSet("jira:issue_created"), + Projects: NewStringSet("myproject"), + IssueTypes: NewStringSet("10001"), + }, + } + } + + setup := func() (*plugintest.API, *Plugin) { api := &plugintest.API{} - p := Plugin{} + p := &Plugin{} p.updateConfig(func(conf *config) { conf.Secret = someSecret @@ -1822,71 +1967,95 @@ func TestCleanupDMSubscriptionsOnDisconnect(t *testing.T) { p.SetAPI(api) p.client = pluginapi.NewClient(api, p.Driver) p.instanceStore = p.getMockInstanceStoreKV(1) + p.userStore = getMockUserStoreKV() + api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return().Maybe() + return api, p + } - api.On("GetDirectChannel", mattermostUserID, botUserID).Return(&model.Channel{ - Id: dmChannelID, - Type: model.ChannelTypeDirect, - }, nil) + t.Run("removes DM subscription and leaves unrelated channel subscription alone", func(t *testing.T) { + api, p := setup() existing := withExistingChannelSubscriptions([]ChannelSubscription{ - { - ID: "sub1______________________", - ChannelID: dmChannelID, - Filters: SubscriptionFilters{ - Events: NewStringSet("jira:issue_created"), - Projects: NewStringSet("myproject"), - IssueTypes: NewStringSet("10001"), - }, - }, - { - ID: "sub2______________________", - ChannelID: otherChannelID, - Filters: SubscriptionFilters{ - Events: NewStringSet("jira:issue_created"), - Projects: NewStringSet("myproject"), - IssueTypes: NewStringSet("10001"), - }, - }, + newSub("sub1______________________", dmChannelID), + newSub("sub2______________________", otherChannelID), }) existingBytes, err := json.Marshal(existing) require.NoError(t, err) - api.On("KVGet", testSubKey).Return(existingBytes, nil) + + api.On("GetChannel", dmChannelID).Return(&model.Channel{Id: dmChannelID, Type: model.ChannelTypeDirect}, nil) + api.On("GetChannel", otherChannelID).Return(&model.Channel{Id: otherChannelID, Type: model.ChannelTypeOpen}, nil) + api.On("GetChannelMember", dmChannelID, mattermostUserID).Return(&model.ChannelMember{ChannelId: dmChannelID, UserId: mattermostUserID}, nil) + api.On("GetChannelMembers", dmChannelID, 0, maxDMGMChannelMembers).Return(model.ChannelMembers{ + {UserId: botUserID}, + {UserId: mattermostUserID}, + }, nil) + api.On("KVSetWithOptions", testSubKey, mock.MatchedBy(func(data []byte) bool { var savedSubs Subscriptions - unmarshalErr := json.Unmarshal(data, &savedSubs) - if unmarshalErr != nil { + if unmarshalErr := json.Unmarshal(data, &savedSubs); unmarshalErr != nil { return false } - _, hasDMSub := savedSubs.Channel.ByID["sub1______________________"] _, hasOtherSub := savedSubs.Channel.ByID["sub2______________________"] return !hasDMSub && hasOtherSub && len(savedSubs.Channel.ByID) == 1 }), mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil) - api.On("LogDebug", mockAnythingOfTypeBatch("string", 11)...).Return() - api.On("LogWarn", mockAnythingOfTypeBatch("string", 10)...).Return() + p.cleanupDMSubscriptionsOnDisconnect(testInstance1.GetID(), mattermostUserID) + + api.AssertExpectations(t) + }) + + t.Run("keeps GM subscription when another member is still connected", func(t *testing.T) { + api, p := setup() + + p.userStore = mockUserStoreKVWithConnected(types.ID(otherMemberID)) + + existing := withExistingChannelSubscriptions([]ChannelSubscription{ + newSub("sub1______________________", gmChannelID), + }) + existingBytes, err := json.Marshal(existing) + require.NoError(t, err) + api.On("KVGet", testSubKey).Return(existingBytes, nil) + + api.On("GetChannel", gmChannelID).Return(&model.Channel{Id: gmChannelID, Type: model.ChannelTypeGroup}, nil) + api.On("GetChannelMember", gmChannelID, mattermostUserID).Return(&model.ChannelMember{ChannelId: gmChannelID, UserId: mattermostUserID}, nil) + api.On("GetChannelMembers", gmChannelID, 0, maxDMGMChannelMembers).Return(model.ChannelMembers{ + {UserId: botUserID}, + {UserId: mattermostUserID}, + {UserId: otherMemberID}, + }, nil) p.cleanupDMSubscriptionsOnDisconnect(testInstance1.GetID(), mattermostUserID) + + api.AssertNotCalled(t, "KVSetWithOptions", mock.Anything, mock.Anything, mock.Anything) }) - t.Run("no-op when DM channel does not exist", func(t *testing.T) { - api := &plugintest.API{} - p := Plugin{} + t.Run("skips channels the disconnecting user is not a member of", func(t *testing.T) { + api, p := setup() - p.updateConfig(func(conf *config) { - conf.Secret = someSecret - conf.botUserID = botUserID + existing := withExistingChannelSubscriptions([]ChannelSubscription{ + newSub("sub1______________________", dmChannelID), }) - p.SetAPI(api) - p.client = pluginapi.NewClient(api, p.Driver) - p.instanceStore = p.getMockInstanceStoreKV(1) + existingBytes, err := json.Marshal(existing) + require.NoError(t, err) + api.On("KVGet", testSubKey).Return(existingBytes, nil) + + api.On("GetChannel", dmChannelID).Return(&model.Channel{Id: dmChannelID, Type: model.ChannelTypeDirect}, nil) + api.On("GetChannelMember", dmChannelID, mattermostUserID).Return(nil, &model.AppError{Message: "not a member"}) + + p.cleanupDMSubscriptionsOnDisconnect(testInstance1.GetID(), mattermostUserID) + + api.AssertNotCalled(t, "GetChannelMembers", mock.Anything, mock.Anything, mock.Anything) + api.AssertNotCalled(t, "KVSetWithOptions", mock.Anything, mock.Anything, mock.Anything) + }) - api.On("GetDirectChannel", mattermostUserID, botUserID).Return(nil, &model.AppError{Message: "channel not found"}) - api.On("LogWarn", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return() + t.Run("no-op when there are no subscriptions for the instance", func(t *testing.T) { + api, p := setup() + api.On("KVGet", testSubKey).Return(nil, nil) p.cleanupDMSubscriptionsOnDisconnect(testInstance1.GetID(), mattermostUserID) - api.AssertNotCalled(t, "KVGet", mock.Anything) + api.AssertNotCalled(t, "GetChannel", mock.Anything) }) } diff --git a/server/webhook_http.go b/server/webhook_http.go index 3b4fb0ab..abb19a19 100644 --- a/server/webhook_http.go +++ b/server/webhook_http.go @@ -90,6 +90,14 @@ func (p *Plugin) httpWebhook(w http.ResponseWriter, r *http.Request, instanceID return respondErr(w, http.StatusBadRequest, err) } + hasConnectedMember, err := p.channelHasConnectedMember(instanceID, channel) + if err != nil { + return respondErr(w, http.StatusInternalServerError, err) + } + if !hasConnectedMember { + return http.StatusOK, nil + } + wh, err := ParseWebhook(bb) if err == ErrWebhookIgnored { return respondErr(w, http.StatusOK, err) diff --git a/server/webhook_http_test.go b/server/webhook_http_test.go index 8f670fb4..0477c83c 100644 --- a/server/webhook_http_test.go +++ b/server/webhook_http_test.go @@ -745,3 +745,71 @@ func TestWebhookHTTP(t *testing.T) { }) } } + +// TestHTTPWebhookDeliveryGuard covers the DM/GM connected-member guard on the legacy +// team+channel webhook URL. +func TestHTTPWebhookDeliveryGuard(t *testing.T) { + const ( + botUserID = "botuser___________________" + connectedUserID = "connecteduser______________" + disconnectedUserID = "disconnecteduser___________" + channelID = "dmchannelaaaaaaaaaaaaaaaa" + ) + + setup := func(t *testing.T) (*plugintest.API, *Plugin) { + api := &plugintest.API{} + p := &Plugin{} + + p.updateConfig(func(conf *config) { + conf.Secret = "thesecret" + conf.botUserID = botUserID + conf.HideDecriptionComment = true + conf.ThreadedJiraCommentSubscriptionDuration = "30" + }) + p.SetAPI(api) + p.client = pluginapi.NewClient(api, p.Driver) + p.instanceStore = p.getMockInstanceStoreKV(1) + p.userStore = getMockUserStoreKV() + + api.On("GetChannelByNameForTeamName", "theteam", "thechannel", false).Return(&model.Channel{ + Id: channelID, + Type: model.ChannelTypeDirect, + }, nil) + + return api, p + } + + t.Run("skips delivery when no DM member is connected", func(t *testing.T) { + api, p := setup(t) + api.On("GetChannelMembers", channelID, 0, maxDMGMChannelMembers).Return(model.ChannelMembers{ + {UserId: botUserID}, + {UserId: disconnectedUserID}, + }, nil) + + status, err := p.httpWebhook(httptest.NewRecorder(), testWebhookRequest("webhook-issue-created.json"), testInstance1.GetID()) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, status) + + api.AssertNotCalled(t, "CreatePost", mock.Anything) + }) + + t.Run("delivers when a DM member is connected", func(t *testing.T) { + api, p := setup(t) + p.userStore = mockUserStoreKVWithConnected(types.ID(connectedUserID)) + + api.On("GetChannelMembers", channelID, 0, maxDMGMChannelMembers).Return(model.ChannelMembers{ + {UserId: botUserID}, + {UserId: connectedUserID}, + }, nil) + api.On("CreatePost", mock.AnythingOfType("*model.Post")).Return(&model.Post{Id: "createdpost1"}, nil) + api.On("KVGet", mock.AnythingOfType("string")).Return(nil, nil) + api.On("KVSetWithOptions", mock.AnythingOfType("string"), mock.Anything, mock.Anything).Return(true, nil) + api.On("LogInfo", mock.Anything, mock.Anything, mock.Anything).Return() + + status, err := p.httpWebhook(httptest.NewRecorder(), testWebhookRequest("webhook-issue-created.json"), testInstance1.GetID()) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, status) + + api.AssertCalled(t, "CreatePost", mock.AnythingOfType("*model.Post")) + }) +} diff --git a/server/webhook_worker.go b/server/webhook_worker.go index 3b08b5ca..a43da22c 100644 --- a/server/webhook_worker.go +++ b/server/webhook_worker.go @@ -86,6 +86,24 @@ func (ww webhookWorker) process(msg *webhookMessage) (err error) { continue } + // DM/GM subscriptions are legacy data; only keep posting to them while at least + // one member is still connected to this instance. + hasConnectedMember, err := ww.p.channelHasConnectedMember(msg.InstanceID, channel) + if err != nil { + ww.p.client.Log.Warn("Failed to check for connected members before posting subscription notification; skipping post", + "ChannelID", channelSubscribed.ChannelID, "InstanceID", string(msg.InstanceID), "Error", err.Error()) + continue + } + if !hasConnectedMember { + ww.p.client.Log.Info("Skipping subscription post to DM/GM channel with no connected members; removing orphaned subscriptions", + "ChannelID", channelSubscribed.ChannelID, "InstanceID", string(msg.InstanceID)) + if removeErr := ww.p.removeSubscriptionsForChannel(msg.InstanceID, channelSubscribed.ChannelID); removeErr != nil { + ww.p.client.Log.Warn("Failed to remove orphaned DM/GM subscriptions", + "ChannelID", channelSubscribed.ChannelID, "InstanceID", string(msg.InstanceID), "Error", removeErr.Error()) + } + continue + } + if _, _, err1 := wh.PostToChannel(ww.p, msg.InstanceID, channelSubscribed.ChannelID, botUserID, channelSubscribed.Name); err1 != nil { ww.p.errorf("WebhookWorker id: %d, error posting to channel, err: %v", ww.id, err1) } diff --git a/server/webhook_worker_test.go b/server/webhook_worker_test.go new file mode 100644 index 00000000..859167b5 --- /dev/null +++ b/server/webhook_worker_test.go @@ -0,0 +1,164 @@ +// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package main + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/plugin/plugintest" + "github.com/mattermost/mattermost/server/public/plugin/plugintest/mock" + "github.com/mattermost/mattermost/server/public/pluginapi" + "github.com/pkg/errors" + "github.com/stretchr/testify/require" + + "github.com/mattermost/mattermost-plugin-jira/server/utils/types" +) + +// TestWebhookWorkerDeliveryGuard covers gating subscription delivery into DM/GM channels +// on a member still being connected, and removing the subscription when none is. The +// "webhook-issue-created.json" fixture skips the notification and watcher code paths, +// leaving the guard as the only thing under test. +func TestWebhookWorkerDeliveryGuard(t *testing.T) { + const ( + botUserID = "botuser___________________" + connectedUserID = "connecteduser______________" + disconnectedUserID = "disconnecteduser___________" + fixtureIssueID = "10040" + fixtureWebhookEvent = "event_created" + ) + + newSub := func(id, channelID string) ChannelSubscription { + return ChannelSubscription{ + ID: id, + ChannelID: channelID, + Filters: SubscriptionFilters{ + Events: NewStringSet(fixtureWebhookEvent), + }, + } + } + + loadWebhookData := func(t *testing.T) []byte { + data, err := getJiraTestData("webhook-issue-created.json") + require.NoError(t, err) + return data + } + + setup := func(t *testing.T, sub ChannelSubscription) (*plugintest.API, *Plugin) { + api := &plugintest.API{} + p := &Plugin{} + + p.updateConfig(func(conf *config) { + conf.Secret = someSecret + conf.botUserID = botUserID + conf.HideDecriptionComment = true + conf.ThreadedJiraCommentSubscriptionDuration = "30" + }) + p.SetAPI(api) + p.client = pluginapi.NewClient(api, p.Driver) + p.instanceStore = p.getMockInstanceStoreKV(1) + p.userStore = getMockUserStoreKV() + + existing := withExistingChannelSubscriptions([]ChannelSubscription{sub}) + existingBytes, err := json.Marshal(existing) + require.NoError(t, err) + api.On("KVGet", testSubKey).Return(existingBytes, nil) + + return api, p + } + + t.Run("skips DM delivery and self-heals when no member is connected", func(t *testing.T) { + channelID := "dmchannelaaaaaaaaaaaaaaaa" + api, p := setup(t, newSub("sub1______________________", channelID)) + + api.On("GetChannel", channelID).Return(&model.Channel{Id: channelID, Type: model.ChannelTypeDirect}, nil) + api.On("GetChannelMembers", channelID, 0, maxDMGMChannelMembers).Return(model.ChannelMembers{ + {UserId: botUserID}, + {UserId: disconnectedUserID}, + }, nil) + api.On("LogInfo", mockAnythingOfTypeBatch("string", 5)...).Return() + api.On("KVSetWithOptions", testSubKey, mock.MatchedBy(func(data []byte) bool { + var savedSubs Subscriptions + if err := json.Unmarshal(data, &savedSubs); err != nil { + return false + } + return len(savedSubs.Channel.ByID) == 0 + }), mock.AnythingOfType("model.PluginKVSetOptions")).Return(true, nil) + + ww := webhookWorker{id: 1, p: p} + err := ww.process(&webhookMessage{InstanceID: testInstance1.GetID(), Data: loadWebhookData(t)}) + require.NoError(t, err) + + api.AssertNotCalled(t, "CreatePost", mock.Anything) + api.AssertExpectations(t) + }) + + t.Run("delivers to DM when a member is still connected", func(t *testing.T) { + channelID := "dmchannelbbbbbbbbbbbbbbbb" + api, p := setup(t, newSub("sub2______________________", channelID)) + + p.userStore = mockUserStoreKVWithConnected(types.ID(connectedUserID)) + + api.On("GetChannel", channelID).Return(&model.Channel{Id: channelID, Type: model.ChannelTypeDirect}, nil) + api.On("GetChannelMembers", channelID, 0, maxDMGMChannelMembers).Return(model.ChannelMembers{ + {UserId: botUserID}, + {UserId: connectedUserID}, + }, nil) + api.On("CreatePost", mock.AnythingOfType("*model.Post")).Return(&model.Post{Id: "createdpost1"}, nil) + api.On("KVSetWithOptions", fmt.Sprintf(ticketRootPostIDKey, fixtureIssueID, channelID), mock.Anything, mock.Anything).Return(true, nil) + + ww := webhookWorker{id: 1, p: p} + err := ww.process(&webhookMessage{InstanceID: testInstance1.GetID(), Data: loadWebhookData(t)}) + require.NoError(t, err) + + api.AssertCalled(t, "CreatePost", mock.AnythingOfType("*model.Post")) + api.AssertNotCalled(t, "KVSetWithOptions", testSubKey, mock.Anything, mock.Anything) + }) + + t.Run("delivers to regular channels without checking membership", func(t *testing.T) { + channelID := "openchannelaaaaaaaaaaaaaa" + api, p := setup(t, newSub("sub3______________________", channelID)) + + api.On("GetChannel", channelID).Return(&model.Channel{Id: channelID, Type: model.ChannelTypeOpen}, nil) + api.On("CreatePost", mock.AnythingOfType("*model.Post")).Return(&model.Post{Id: "createdpost2"}, nil) + api.On("KVSetWithOptions", fmt.Sprintf(ticketRootPostIDKey, fixtureIssueID, channelID), mock.Anything, mock.Anything).Return(true, nil) + + ww := webhookWorker{id: 1, p: p} + err := ww.process(&webhookMessage{InstanceID: testInstance1.GetID(), Data: loadWebhookData(t)}) + require.NoError(t, err) + + api.AssertCalled(t, "CreatePost", mock.AnythingOfType("*model.Post")) + api.AssertNotCalled(t, "GetChannelMembers", mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("keeps the subscription when the connection lookup fails", func(t *testing.T) { + channelID := "dmchannelcccccccccccccccc" + api, p := setup(t, newSub("sub4______________________", channelID)) + p.userStore = failingConnectionUserStore{UserStore: p.userStore} + + api.On("GetChannel", channelID).Return(&model.Channel{Id: channelID, Type: model.ChannelTypeDirect}, nil) + api.On("GetChannelMembers", channelID, 0, maxDMGMChannelMembers).Return(model.ChannelMembers{ + {UserId: botUserID}, + {UserId: disconnectedUserID}, + }, nil) + api.On("LogWarn", mockAnythingOfTypeBatch("string", 7)...).Return() + + ww := webhookWorker{id: 1, p: p} + err := ww.process(&webhookMessage{InstanceID: testInstance1.GetID(), Data: loadWebhookData(t)}) + require.NoError(t, err) + + api.AssertNotCalled(t, "CreatePost", mock.Anything) + api.AssertNotCalled(t, "KVSetWithOptions", testSubKey, mock.Anything, mock.Anything) + }) +} + +type failingConnectionUserStore struct { + UserStore +} + +func (failingConnectionUserStore) LoadConnection(types.ID, types.ID) (*Connection, error) { + return nil, errors.New("kv store unavailable") +}