diff --git a/server/channels/app/notification_push.go b/server/channels/app/notification_push.go index cd4cfd0d12..7de7ec49db 100644 --- a/server/channels/app/notification_push.go +++ b/server/channels/app/notification_push.go @@ -600,6 +600,19 @@ func (a *App) getMobileAppSessions(userID string) ([]*model.Session, *model.AppE } func (a *App) ShouldSendPushNotification(user *model.User, channelNotifyProps model.StringMap, wasMentioned bool, status *model.Status, post *model.Post, isGM bool) bool { + if user.IsBot { + a.CountNotificationReason(model.NotificationStatusNotSent, model.NotificationTypePush, model.NotificationReasonRecipientIsBot, model.NotificationNoPlatform) + a.NotificationsLog().Debug("Notification not sent - recipient is bot", + mlog.String("type", model.NotificationTypePush), + mlog.String("post_id", post.Id), + mlog.String("status", model.NotificationStatusNotSent), + mlog.String("reason", model.NotificationReasonRecipientIsBot), + mlog.String("sender_id", post.UserId), + mlog.String("receiver_id", user.Id), + ) + return false + } + if prop := post.GetProp(model.PostPropsForceNotification); prop != nil && prop != "" { return true } diff --git a/server/channels/app/notification_push_test.go b/server/channels/app/notification_push_test.go index 5103719e1d..9191b59635 100644 --- a/server/channels/app/notification_push_test.go +++ b/server/channels/app/notification_push_test.go @@ -1140,6 +1140,50 @@ func TestShouldSendPushNotifications(t *testing.T) { result := th.App.ShouldSendPushNotification(user, channelNotifyProps, false, status, post, false) assert.False(t, result) }) + + t.Run("should return false if recipient is a bot", func(t *testing.T) { + botUser := &model.User{Id: model.NewId(), Email: "bot@test.com", IsBot: true, NotifyProps: make(map[string]string)} + botUser.NotifyProps[model.PushNotifyProp] = model.UserNotifyAll + + post := &model.Post{UserId: model.NewId(), ChannelId: model.NewId()} + + channelNotifyProps := map[string]string{model.PushNotifyProp: model.ChannelNotifyAll} + + status := &model.Status{UserId: botUser.Id, Status: model.StatusOnline, Manual: false, LastActivityAt: model.GetMillis(), ActiveChannel: ""} + + result := th.App.ShouldSendPushNotification(botUser, channelNotifyProps, true, status, post, false) + assert.False(t, result) + }) + + t.Run("should return false if recipient is a bot even with force notification", func(t *testing.T) { + botUser := &model.User{Id: model.NewId(), Email: "bot@test.com", IsBot: true, NotifyProps: make(map[string]string)} + botUser.NotifyProps[model.PushNotifyProp] = model.UserNotifyAll + + post := &model.Post{UserId: model.NewId(), ChannelId: model.NewId()} + post.AddProp(model.PostPropsForceNotification, model.NewId()) + + channelNotifyProps := map[string]string{model.PushNotifyProp: model.ChannelNotifyAll} + + status := &model.Status{UserId: botUser.Id, Status: model.StatusOnline, Manual: false, LastActivityAt: model.GetMillis(), ActiveChannel: ""} + + result := th.App.ShouldSendPushNotification(botUser, channelNotifyProps, true, status, post, false) + assert.False(t, result) + }) + + t.Run("should return true for regular user even when post author is a bot", func(t *testing.T) { + botId := model.NewId() + regularUser := &model.User{Id: model.NewId(), Email: "user@test.com", IsBot: false, NotifyProps: make(map[string]string)} + regularUser.NotifyProps[model.PushNotifyProp] = model.UserNotifyAll + + post := &model.Post{UserId: botId, ChannelId: model.NewId()} + + channelNotifyProps := map[string]string{model.PushNotifyProp: model.ChannelNotifyAll} + + status := &model.Status{UserId: regularUser.Id, Status: model.StatusOnline, Manual: false, LastActivityAt: model.GetMillis(), ActiveChannel: ""} + + result := th.App.ShouldSendPushNotification(regularUser, channelNotifyProps, true, status, post, false) + assert.True(t, result) + }) } // testPushNotificationHandler is an HTTP handler to record push notifications diff --git a/server/channels/app/post_persistent_notification.go b/server/channels/app/post_persistent_notification.go index da84bfa492..a784cc790c 100644 --- a/server/channels/app/post_persistent_notification.go +++ b/server/channels/app/post_persistent_notification.go @@ -171,6 +171,18 @@ func (a *App) forEachPersistentNotificationPost(posts []*model.Post, fn func(pos } profileMap := channelProfileMap[channel.Id] + // Ensure the sender is always in the profile map: for example, system admins can post + // without being a member. + if _, ok := profileMap[post.UserId]; !ok { + var sender *model.User + sender, err = a.Srv().Store().User().Get(context.Background(), post.UserId) + if err != nil { + return errors.Wrapf(err, "failed to get profile for sender user %s for post %s", post.UserId, post.Id) + } + + profileMap[post.UserId] = sender + } + mentions := &MentionResults{} // In DMs, only the "other" user can be mentioned if channel.Type == model.ChannelTypeDirect { @@ -229,15 +241,12 @@ func (a *App) persistentNotificationsAuxiliaryData(channelsMap map[string]*model } channelKeywords[c.Id] = make(MentionKeywords, len(profileMap)) - validProfileMap := make(map[string]*model.User, len(profileMap)) for userID, user := range profileMap { - if user.IsBot { - continue + if !user.IsBot { + channelKeywords[c.Id].AddUserKeyword(userID, "@"+user.Username) } - validProfileMap[userID] = user - channelKeywords[c.Id].AddUserKeyword(userID, "@"+user.Username) } - channelProfileMap[c.Id] = validProfileMap + channelProfileMap[c.Id] = profileMap } return channelGroupMap, channelProfileMap, channelKeywords, channelNotifyProps, nil } diff --git a/server/channels/app/post_persistent_notification_test.go b/server/channels/app/post_persistent_notification_test.go index c83cab8512..929d55e43b 100644 --- a/server/channels/app/post_persistent_notification_test.go +++ b/server/channels/app/post_persistent_notification_test.go @@ -5,10 +5,12 @@ package app import ( "testing" + "time" "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/v8/channels/store" storemocks "github.com/mattermost/mattermost/server/v8/channels/store/storetest/mocks" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -223,3 +225,106 @@ func TestSendPersistentNotifications(t *testing.T) { err := th.App.SendPersistentNotifications() require.NoError(t, err) } + +func TestSendPersistentNotificationsBotSender(t *testing.T) { + mainHelper.Parallel(t) + t.Run("should send notification when bot is sender", func(t *testing.T) { + th := Setup(t).InitBasic() + + bot, appErr := th.App.CreateBot(th.Context, &model.Bot{ + Username: "testbot", + DisplayName: "Test Bot", + OwnerId: th.BasicUser.Id, + }) + require.Nil(t, appErr) + + botUser, appErr := th.App.GetUser(bot.UserId) + require.Nil(t, appErr) + + _, _, appErr = th.App.AddUserToTeam(th.Context, th.BasicTeam.Id, botUser.Id, "") + require.Nil(t, appErr) + + _, appErr = th.App.AddUserToChannel(th.Context, botUser, th.BasicChannel, false) + require.Nil(t, appErr) + + _, appErr = th.App.AddUserToChannel(th.Context, th.BasicUser2, th.BasicChannel, false) + require.Nil(t, appErr) + + post := &model.Post{ + UserId: bot.UserId, + ChannelId: th.BasicChannel.Id, + Message: "test " + "@" + th.BasicUser2.Username, + Metadata: &model.PostMetadata{ + Priority: &model.PostPriority{ + Priority: model.NewPointer(model.PostPriorityUrgent), + PersistentNotifications: model.NewPointer(true), + }, + }, + // Simulate old timestamp so persistent notifications are sent right away + CreateAt: time.Now().Add(-5 * time.Minute).UnixMilli(), + } + post, appErr = th.App.CreatePost(th.Context, post, th.BasicChannel, model.CreatePostFlags{}) + require.Nil(t, appErr) + + assert.EventuallyWithT(t, func(c *assert.CollectT) { + err := th.App.SendPersistentNotifications() + require.NoError(t, err) + + persistentPostNotification, err := th.App.Srv().Store().PostPersistentNotification().GetSingle(post.Id) + require.NoError(c, err) + require.NotNil(c, persistentPostNotification) + assert.Greater(c, persistentPostNotification.SentCount, int16(0)) + }, 5*time.Second, 100*time.Millisecond) + }) +} + +func TestSendPersistentNotificationsBotSenderNotInChannel(t *testing.T) { + mainHelper.Parallel(t) + t.Run("should send notification when bot sender is not a channel member", func(t *testing.T) { + th := Setup(t).InitBasic() + + bot, appErr := th.App.CreateBot(th.Context, &model.Bot{ + Username: "testbot", + DisplayName: "Test Bot", + OwnerId: th.BasicUser.Id, + }) + require.Nil(t, appErr) + + botUser, appErr := th.App.GetUser(bot.UserId) + require.Nil(t, appErr) + + // Make the bot a system admin so it can post to channels it's not a member of + _, appErr = th.App.UpdateUserRoles(th.Context, botUser.Id, model.SystemUserRoleId+" "+model.SystemAdminRoleId, false) + require.Nil(t, appErr) + + _, appErr = th.App.AddUserToChannel(th.Context, th.BasicUser2, th.BasicChannel, false) + require.Nil(t, appErr) + + // Note: bot is NOT added to the team or channel + + post := &model.Post{ + UserId: bot.UserId, + ChannelId: th.BasicChannel.Id, + Message: "test " + "@" + th.BasicUser2.Username, + Metadata: &model.PostMetadata{ + Priority: &model.PostPriority{ + Priority: model.NewPointer(model.PostPriorityUrgent), + PersistentNotifications: model.NewPointer(true), + }, + }, + CreateAt: time.Now().Add(-5 * time.Minute).UnixMilli(), + } + post, appErr = th.App.CreatePost(th.Context, post, th.BasicChannel, model.CreatePostFlags{SetOnline: true}) + require.Nil(t, appErr) + + assert.EventuallyWithT(t, func(c *assert.CollectT) { + err := th.App.SendPersistentNotifications() + require.NoError(t, err) + + persistentPostNotification, err := th.App.Srv().Store().PostPersistentNotification().GetSingle(post.Id) + require.NoError(c, err) + require.NotNil(c, persistentPostNotification) + assert.Greater(c, persistentPostNotification.SentCount, int16(0)) + }, 5*time.Second, 100*time.Millisecond) + }) +} diff --git a/server/public/model/notification.go b/server/public/model/notification.go index da23f132d1..8da00a07bb 100644 --- a/server/public/model/notification.go +++ b/server/public/model/notification.go @@ -40,4 +40,5 @@ const ( NotificationReasonTooManyUsersInChannel NotificationReason = "too_many_users_in_channel" NotificationReasonResolvePersistentNotificationError NotificationReason = "resolve_persistent_notification_error" NotificationReasonMissingThreadMembership NotificationReason = "missing_thread_membership" + NotificationReasonRecipientIsBot NotificationReason = "recipient_is_bot" )