MM-65575: Fix server panic when bot posts trigger persistent notifications (#34174) (#34778)

* reproduce panic with test

* allow bots in the profile map

* explicitly prevent sending notifications to bots

* persistent notifications: handle senders not in the channel

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
 	}

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

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
 }

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)
+	})
+}

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"
 )