diff --git a/server/channels/app/bot.go b/server/channels/app/bot.go index 09c036a6904..a149801859d 100644 --- a/server/channels/app/bot.go +++ b/server/channels/app/bot.go @@ -353,14 +353,22 @@ func (a *App) GetBots(rctx request.CTX, options *model.BotGetOptions) (model.Bot return bots, nil } -// IsBotOwnedByCurrentUserOrPlugin checks if the given user ID is a bot owned by the current session's user or by a plugin. -func (a *App) IsBotOwnedByCurrentUserOrPlugin(rctx request.CTX, userID string) (bool, *model.AppError) { +// IsBotExemptFromDMRestrictions checks if the given user ID is a bot that is +// exempt from the RestrictDirectMessage=team enforcement. This includes the +// system bot, bots owned by the current session's user, and plugin-owned bots. +func (a *App) IsBotExemptFromDMRestrictions(rctx request.CTX, userID string) (bool, *model.AppError) { bot, appErr := a.GetBot(rctx, userID, false) if appErr != nil { return false, appErr } - if bot.OwnerId == rctx.Session().UserId { + // The system bot must be able to send messages to any user regardless of + // team membership (e.g. push notification tests, post reminders, etc.) + if bot.Username == model.BotSystemBotUsername { + return true, nil + } + + if session := rctx.Session(); session != nil && bot.OwnerId == session.UserId { return true, nil } @@ -371,7 +379,7 @@ func (a *App) IsBotOwnedByCurrentUserOrPlugin(rctx request.CTX, userID string) ( availablePlugins, err := pluginsEnvironment.Available() if err != nil { - return false, model.NewAppError("IsBotOwnedByCurrentUserOrPlugin", "app.plugin.get_plugins.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + return false, model.NewAppError("IsBotExemptFromDMRestrictions", "app.plugin.get_plugins.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } pluginIDs := make(map[string]bool, len(availablePlugins)) diff --git a/server/channels/app/bot_test.go b/server/channels/app/bot_test.go index 4a82298fea6..51819f89a04 100644 --- a/server/channels/app/bot_test.go +++ b/server/channels/app/bot_test.go @@ -980,7 +980,7 @@ func TestGetSystemBot(t *testing.T) { }) } -func TestIsBotOwnedByCurrentUserOrPlugin(t *testing.T) { +func TestIsBotExemptFromDMRestrictions(t *testing.T) { mainHelper.Parallel(t) t.Run("bot owned by current user", func(t *testing.T) { th := Setup(t).InitBasic(t) @@ -1003,7 +1003,7 @@ func TestIsBotOwnedByCurrentUserOrPlugin(t *testing.T) { require.Nil(t, err) rctx := th.Context.WithSession(session) - owned, appErr := th.App.IsBotOwnedByCurrentUserOrPlugin(rctx, bot.UserId) + owned, appErr := th.App.IsBotExemptFromDMRestrictions(rctx, bot.UserId) require.Nil(t, appErr) assert.True(t, owned) }) @@ -1029,7 +1029,7 @@ func TestIsBotOwnedByCurrentUserOrPlugin(t *testing.T) { require.Nil(t, err) rctx := th.Context.WithSession(session) - owned, appErr := th.App.IsBotOwnedByCurrentUserOrPlugin(rctx, bot.UserId) + owned, appErr := th.App.IsBotExemptFromDMRestrictions(rctx, bot.UserId) require.Nil(t, appErr) assert.False(t, owned) }) @@ -1044,7 +1044,7 @@ func TestIsBotOwnedByCurrentUserOrPlugin(t *testing.T) { require.Nil(t, err) rctx := th.Context.WithSession(session) - owned, appErr := th.App.IsBotOwnedByCurrentUserOrPlugin(rctx, model.NewId()) + owned, appErr := th.App.IsBotExemptFromDMRestrictions(rctx, model.NewId()) require.NotNil(t, appErr) assert.False(t, owned) require.Equal(t, "store.sql_bot.get.missing.app_error", appErr.Id) @@ -1072,7 +1072,7 @@ func TestIsBotOwnedByCurrentUserOrPlugin(t *testing.T) { require.Nil(t, err) rctx := th.Context.WithSession(session) - owned, appErr := th.App.IsBotOwnedByCurrentUserOrPlugin(rctx, bot.UserId) + owned, appErr := th.App.IsBotExemptFromDMRestrictions(rctx, bot.UserId) require.Nil(t, appErr) assert.False(t, owned) }) @@ -1118,8 +1118,32 @@ func TestIsBotOwnedByCurrentUserOrPlugin(t *testing.T) { require.Nil(t, err) rctx := th.Context.WithSession(session) - owned, appErr := th.App.IsBotOwnedByCurrentUserOrPlugin(rctx, bot.UserId) + owned, appErr := th.App.IsBotExemptFromDMRestrictions(rctx, bot.UserId) require.Nil(t, appErr) assert.True(t, owned) }) + + t.Run("system bot is always exempt regardless of session", func(t *testing.T) { + th := Setup(t).InitBasic(t) + + systemBot, appErr := th.App.GetSystemBot(th.Context) + require.Nil(t, appErr) + + // Exempt even with an empty context (background job with no session) + exempt, appErr := th.App.IsBotExemptFromDMRestrictions(th.Context, systemBot.UserId) + require.Nil(t, appErr) + assert.True(t, exempt) + + // Exempt even when the session belongs to an unrelated non-admin user + session, err := th.App.CreateSession(th.Context, &model.Session{ + UserId: th.BasicUser.Id, + Roles: th.BasicUser.GetRawRoles(), + }) + require.Nil(t, err) + rctx := th.Context.WithSession(session) + + exempt, appErr = th.App.IsBotExemptFromDMRestrictions(rctx, systemBot.UserId) + require.Nil(t, appErr) + assert.True(t, exempt) + }) } diff --git a/server/channels/app/channel.go b/server/channels/app/channel.go index 63c53016abf..79f0c795982 100644 --- a/server/channels/app/channel.go +++ b/server/channels/app/channel.go @@ -329,21 +329,21 @@ func (a *App) GetOrCreateDirectChannel(rctx request.CTX, userID, otherUserID str if err != nil { return nil, err } - var isPluginOwnedBot bool + var isBotExempt bool for _, user := range users { if user.IsBot { - isOwnedByCurrentUserOrPlugin, err := a.IsBotOwnedByCurrentUserOrPlugin(rctx, user.Id) + exempt, err := a.IsBotExemptFromDMRestrictions(rctx, user.Id) if err != nil { return nil, err } - if isOwnedByCurrentUserOrPlugin { - isPluginOwnedBot = true + if exempt { + isBotExempt = true break } } } - // if one of the users is a bot, don't restrict to team members - if !isPluginOwnedBot { + // if one of the users is an exempt bot, don't restrict to team members + if !isBotExempt { commonTeamIDs, err := a.GetCommonTeamIDsForTwoUsers(userID, otherUserID) if err != nil { return nil, err @@ -3815,11 +3815,11 @@ func (a *App) getDirectOrGroupMessageMembersCommonTeams(rctx request.CTX, reques userIDs := make([]string, 0, len(users)) for _, user := range users { if user.IsBot { - isOwnedByCurrentUserOrPlugin, err := a.IsBotOwnedByCurrentUserOrPlugin(rctx, user.Id) + exempt, err := a.IsBotExemptFromDMRestrictions(rctx, user.Id) if err != nil { return nil, err } - if isOwnedByCurrentUserOrPlugin { + if exempt { continue } } diff --git a/server/channels/app/channel_test.go b/server/channels/app/channel_test.go index c9f2cf39735..0960459b296 100644 --- a/server/channels/app/channel_test.go +++ b/server/channels/app/channel_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/require" "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/request" "github.com/mattermost/mattermost/server/v8/channels/store/storetest/mocks" ) @@ -538,7 +539,7 @@ func TestGetOrCreateDirectChannel(t *testing.T) { cfg.TeamSettings.RestrictDirectMessage = &setting }) - // Create a session for the bot owner so IsBotOwnedByCurrentUserOrPlugin can work + // Create a session for the bot owner so IsBotExemptFromDMRestrictions can work session, err := th.App.CreateSession(th.Context, &model.Session{ UserId: th.BasicUser.Id, Roles: th.BasicUser.GetRawRoles(), @@ -557,6 +558,35 @@ func TestGetOrCreateDirectChannel(t *testing.T) { require.Nil(t, appErr) }) + t.Run("System bot can DM any user with RestrictDirectMessage=team (MM-67314)", func(t *testing.T) { + th.App.UpdateConfig(func(cfg *model.Config) { + setting := model.DirectMessageTeam + cfg.TeamSettings.RestrictDirectMessage = &setting + }) + + systemBot, appErr := th.App.GetSystemBot(th.Context) + require.Nil(t, appErr) + + // Simulate a background job (e.g. CheckPostReminders) that uses an empty session. + // The system bot is on no teams, so without the fix this would return an error. + emptyCtx := request.EmptyContext(th.App.Log()) + channel, appErr := th.App.GetOrCreateDirectChannel(emptyCtx, user1.Id, systemBot.UserId) + require.Nil(t, appErr) + require.NotNil(t, channel) + + // Simulate a regular user triggering SendTestMessage from a non-admin session. + session, err := th.App.CreateSession(th.Context, &model.Session{ + UserId: user1.Id, + Roles: user1.GetRawRoles(), + }) + require.Nil(t, err) + rctx := th.Context.WithSession(session) + + channel, appErr = th.App.GetOrCreateDirectChannel(rctx, user1.Id, systemBot.UserId) + require.Nil(t, appErr) + require.NotNil(t, channel) + }) + t.Run("User from other team cannot create with restriction", func(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { setting := model.DirectMessageTeam