diff --git a/mattermost-plugin/server/notifications.go b/mattermost-plugin/server/notifications.go index 71a53e887..9340787b7 100644 --- a/mattermost-plugin/server/notifications.go +++ b/mattermost-plugin/server/notifications.go @@ -30,35 +30,43 @@ type notifyBackendParams struct { cfg *config.Configuration client *pluginapi.Client permissions permissions.PermissionsService + store store.Store + wsAdapter ws.Adapter serverRoot string logger *mlog.Logger } func createMentionsNotifyBackend(params notifyBackendParams) (*notifymentions.Backend, error) { - delivery, err := createDelivery(params.client, params.serverRoot, params.permissions) + delivery, err := createDelivery(params.client, params.serverRoot) if err != nil { return nil, err } - backend := notifymentions.New(delivery, params.permissions, params.logger) + backendParams := notifymentions.BackendParams{ + Store: params.store, + Permissions: params.permissions, + Delivery: delivery, + WSAdapter: params.wsAdapter, + Logger: params.logger, + } + + backend := notifymentions.New(backendParams) return backend, nil } -func createSubscriptionsNotifyBackend(params notifyBackendParams, store store.Store, - wsPluginAdapter ws.PluginAdapterInterface) (*notifysubscriptions.Backend, error) { - // - delivery, err := createDelivery(params.client, params.serverRoot, params.permissions) +func createSubscriptionsNotifyBackend(params notifyBackendParams) (*notifysubscriptions.Backend, error) { + delivery, err := createDelivery(params.client, params.serverRoot) if err != nil { return nil, err } backendParams := notifysubscriptions.BackendParams{ ServerRoot: params.serverRoot, - Store: store, + Store: params.store, Permissions: params.permissions, Delivery: delivery, - WSAdapter: wsPluginAdapter, + WSAdapter: params.wsAdapter, Logger: params.logger, NotifyFreqCardSeconds: params.cfg.NotifyFreqCardSeconds, NotifyFreqBoardSeconds: params.cfg.NotifyFreqBoardSeconds, @@ -68,7 +76,7 @@ func createSubscriptionsNotifyBackend(params notifyBackendParams, store store.St return backend, nil } -func createDelivery(client *pluginapi.Client, serverRoot string, permissions permissions.PermissionsService) (*plugindelivery.PluginDelivery, error) { +func createDelivery(client *pluginapi.Client, serverRoot string) (*plugindelivery.PluginDelivery, error) { bot := &model.Bot{ Username: botUsername, DisplayName: botDisplayname, @@ -81,7 +89,7 @@ func createDelivery(client *pluginapi.Client, serverRoot string, permissions per pluginAPI := &pluginAPIAdapter{client: client} - return plugindelivery.New(botID, serverRoot, pluginAPI, permissions), nil + return plugindelivery.New(botID, serverRoot, pluginAPI), nil } type pluginAPIAdapter struct { diff --git a/mattermost-plugin/server/plugin.go b/mattermost-plugin/server/plugin.go index 9d2fc2cf8..685c4cc97 100644 --- a/mattermost-plugin/server/plugin.go +++ b/mattermost-plugin/server/plugin.go @@ -122,7 +122,9 @@ func (p *Plugin) OnActivate() error { backendParams := notifyBackendParams{ cfg: cfg, client: client, + store: db, permissions: permissionsService, + wsAdapter: p.wsPluginAdapter, serverRoot: baseURL + "/boards", logger: logger, } @@ -135,7 +137,7 @@ func (p *Plugin) OnActivate() error { } notifyBackends = append(notifyBackends, mentionsBackend) - subscriptionsBackend, err2 := createSubscriptionsNotifyBackend(backendParams, db, p.wsPluginAdapter) + subscriptionsBackend, err2 := createSubscriptionsNotifyBackend(backendParams) if err2 != nil { return fmt.Errorf("error creating subscription notifications backend: %w", err2) } @@ -209,7 +211,7 @@ func (p *Plugin) createBoardsConfig(mmconfig mmModel.Config, baseURL string, ser featureFlags := parseFeatureFlags(mmconfig.FeatureFlags.ToMap()) return &config.Configuration{ - ServerRoot: baseURL + "/plugins/focalboard", + ServerRoot: baseURL, Port: -1, DBType: *mmconfig.SqlSettings.DriverName, DBConfigString: *mmconfig.SqlSettings.DataSource, diff --git a/server/app/blocks.go b/server/app/blocks.go index 8e7825cc3..7ceb0104d 100644 --- a/server/app/blocks.go +++ b/server/app/blocks.go @@ -356,6 +356,15 @@ func (a *App) notifyBlockChanged(action notify.Action, block *model.Block, oldBl return } + boardMember, _ := a.GetMemberForBoard(board.ID, modifiedByID) + if boardMember == nil { + // create temporary guest board member + boardMember = &model.BoardMember{ + BoardID: board.ID, + UserID: modifiedByID, + } + } + evt := notify.BlockChangeEvent{ Action: action, TeamID: board.TeamID, @@ -363,7 +372,7 @@ func (a *App) notifyBlockChanged(action notify.Action, block *model.Block, oldBl Card: card, BlockChanged: block, BlockOld: oldBlock, - ModifiedByID: modifiedByID, + ModifiedBy: boardMember, } a.notifications.BlockChanged(evt) } diff --git a/server/app/boards.go b/server/app/boards.go index 21b3c8350..70547b78a 100644 --- a/server/app/boards.go +++ b/server/app/boards.go @@ -240,6 +240,10 @@ func (a *App) GetMembersForUser(userID string) ([]*model.BoardMember, error) { return a.store.GetMembersForUser(userID) } +func (a *App) GetMemberForBoard(boardID string, userID string) (*model.BoardMember, error) { + return a.store.GetMemberForBoard(boardID, userID) +} + func (a *App) AddMemberToBoard(member *model.BoardMember) (*model.BoardMember, error) { board, err := a.store.GetBoard(member.BoardID) if errors.Is(err, sql.ErrNoRows) { diff --git a/server/services/notify/notifymentions/delivery.go b/server/services/notify/notifymentions/delivery.go index 271f37334..129dde463 100644 --- a/server/services/notify/notifymentions/delivery.go +++ b/server/services/notify/notifymentions/delivery.go @@ -3,11 +3,17 @@ package notifymentions -import "github.com/mattermost/focalboard/server/services/notify" +import ( + "github.com/mattermost/focalboard/server/services/notify" + + mm_model "github.com/mattermost/mattermost-server/v6/model" +) // MentionDelivery provides an interface for delivering @mention notifications to other systems, such as // channels server via plugin API. // On success the user id of the user mentioned is returned. type MentionDelivery interface { - MentionDeliver(mentionUsername string, extract string, evt notify.BlockChangeEvent) (string, error) + MentionDeliver(mentionedUser *mm_model.User, extract string, evt notify.BlockChangeEvent) (string, error) + UserByUsername(mentionUsername string) (*mm_model.User, error) + IsErrNotFound(err error) bool } diff --git a/server/services/notify/notifymentions/mentions_backend.go b/server/services/notify/notifymentions/mentions_backend.go index 7fe08c6b8..0b05d9b21 100644 --- a/server/services/notify/notifymentions/mentions_backend.go +++ b/server/services/notify/notifymentions/mentions_backend.go @@ -4,12 +4,14 @@ package notifymentions import ( + "errors" "fmt" "sync" "github.com/mattermost/focalboard/server/model" "github.com/mattermost/focalboard/server/services/notify" "github.com/mattermost/focalboard/server/services/permissions" + "github.com/mattermost/focalboard/server/ws" "github.com/wiggin77/merror" "github.com/mattermost/mattermost-server/v6/shared/mlog" @@ -19,25 +21,41 @@ const ( backendName = "notifyMentions" ) +var ( + ErrMentionPermission = errors.New("mention not permitted") +) + type MentionListener interface { OnMention(userID string, evt notify.BlockChangeEvent) } +type BackendParams struct { + Store Store + Permissions permissions.PermissionsService + Delivery MentionDelivery + WSAdapter ws.Adapter + Logger *mlog.Logger +} + // Backend provides the notification backend for @mentions. type Backend struct { - delivery MentionDelivery + store Store permissions permissions.PermissionsService + delivery MentionDelivery + wsAdapter ws.Adapter logger *mlog.Logger mux sync.RWMutex listeners []MentionListener } -func New(delivery MentionDelivery, permissions permissions.PermissionsService, logger *mlog.Logger) *Backend { +func New(params BackendParams) *Backend { return &Backend{ - delivery: delivery, - permissions: permissions, - logger: logger, + store: params.Store, + permissions: params.Permissions, + delivery: params.Delivery, + wsAdapter: params.WSAdapter, + logger: params.Logger, } } @@ -110,7 +128,7 @@ func (b *Backend) BlockChanged(evt notify.BlockChangeEvent) error { extract := extractText(evt.BlockChanged.Title, username, newLimits()) - userID, err := b.delivery.MentionDeliver(username, extract, evt) + userID, err := b.deliverMentionNotification(username, extract, evt) if err != nil { merr.Append(fmt.Errorf("cannot deliver notification for @%s: %w", username, err)) } @@ -141,3 +159,79 @@ func safeCallListener(listener MentionListener, userID string, evt notify.BlockC }() listener.OnMention(userID, evt) } + +func (b *Backend) deliverMentionNotification(username string, extract string, evt notify.BlockChangeEvent) (string, error) { + mentionedUser, err := b.delivery.UserByUsername(username) + if err != nil { + if b.delivery.IsErrNotFound(err) { + // not really an error; could just be someone typed "@sometext" + return "", nil + } else { + return "", fmt.Errorf("cannot lookup mentioned user: %w", err) + } + } + + if evt.ModifiedBy == nil { + return "", fmt.Errorf("invalid user cannot mention: %w", ErrMentionPermission) + } + + if evt.Board.Type == model.BoardTypeOpen { + // public board rules: + // - admin, editor, commenter: can mention anyone on team (mentioned users are automatically added to board) + // - guest: can mention board members + switch { + case evt.ModifiedBy.SchemeAdmin, evt.ModifiedBy.SchemeEditor, evt.ModifiedBy.SchemeCommenter: + if !b.permissions.HasPermissionToTeam(mentionedUser.Id, evt.TeamID, model.PermissionViewTeam) { + return "", fmt.Errorf("%s cannot mention non-team member %s : %w", evt.ModifiedBy.UserID, mentionedUser.Id, ErrMentionPermission) + } + // add mentioned user to board (if not already a member) + member, err := b.store.GetMemberForBoard(evt.Board.ID, mentionedUser.Id) + if member == nil || b.store.IsErrNotFound(err) { + // currently all memberships are created as editors by default + newBoardMember := &model.BoardMember{ + UserID: mentionedUser.Id, + BoardID: evt.Board.ID, + SchemeEditor: true, + } + if member, err = b.store.SaveMember(newBoardMember); err != nil { + return "", fmt.Errorf("cannot add mentioned user %s to board %s: %w", mentionedUser.Id, evt.Board.ID, err) + } + b.logger.Debug("auto-added mentioned user to board", + mlog.String("user_id", mentionedUser.Id), + mlog.String("board_id", evt.Board.ID), + mlog.String("board_type", string(evt.Board.Type)), + ) + b.wsAdapter.BroadcastMemberChange(evt.TeamID, evt.Board.ID, member) + } else { + b.logger.Debug("skipping auto-add mentioned user to board; already a member", + mlog.String("user_id", mentionedUser.Id), + mlog.String("board_id", evt.Board.ID), + mlog.String("board_type", string(evt.Board.Type)), + ) + } + case evt.ModifiedBy.SchemeViewer: + // viewer should not have gotten this far since they cannot add text to a card + return "", fmt.Errorf("%s (viewer) cannot mention user %s: %w", evt.ModifiedBy.UserID, mentionedUser.Id, ErrMentionPermission) + default: + // this is a guest + if !b.permissions.HasPermissionToBoard(mentionedUser.Id, evt.Board.ID, model.PermissionViewBoard) { + return "", fmt.Errorf("%s cannot mention non-board member %s : %w", evt.ModifiedBy.UserID, mentionedUser.Id, ErrMentionPermission) + } + } + } else { + // private board rules: + // - admin, editor, commenter, guest: can mention board members + switch { + case evt.ModifiedBy.SchemeViewer: + // viewer should not have gotten this far since they cannot add text to a card + return "", fmt.Errorf("%s (viewer) cannot mention user %s: %w", evt.ModifiedBy.UserID, mentionedUser.Id, ErrMentionPermission) + default: + // everyone else can mention board members + if !b.permissions.HasPermissionToBoard(mentionedUser.Id, evt.Board.ID, model.PermissionViewBoard) { + return "", fmt.Errorf("%s cannot mention non-board member %s : %w", evt.ModifiedBy.UserID, mentionedUser.Id, ErrMentionPermission) + } + } + } + + return b.delivery.MentionDeliver(mentionedUser, extract, evt) +} diff --git a/server/services/notify/notifymentions/store.go b/server/services/notify/notifymentions/store.go new file mode 100644 index 000000000..d0d484a18 --- /dev/null +++ b/server/services/notify/notifymentions/store.go @@ -0,0 +1,16 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. +package notifymentions + +import "github.com/mattermost/focalboard/server/model" + +type Store interface { + GetUserByID(userID string) (*model.User, error) + + GetMemberForBoard(boardID, userID string) (*model.BoardMember, error) + SaveMember(bm *model.BoardMember) (*model.BoardMember, error) + + CreateSubscription(sub *model.Subscription) (*model.Subscription, error) + + IsErrNotFound(err error) bool +} diff --git a/server/services/notify/notifysubscriptions/store.go b/server/services/notify/notifysubscriptions/store.go index 4de0f1e9f..a13dc178a 100644 --- a/server/services/notify/notifysubscriptions/store.go +++ b/server/services/notify/notifysubscriptions/store.go @@ -18,6 +18,7 @@ type Store interface { GetUserByID(userID string) (*model.User, error) GetMemberForBoard(boardID, userID string) (*model.BoardMember, error) + SaveMember(bm *model.BoardMember) (*model.BoardMember, error) CreateSubscription(sub *model.Subscription) (*model.Subscription, error) GetSubscribersForBlock(blockID string) ([]*model.Subscriber, error) diff --git a/server/services/notify/notifysubscriptions/subscriptions_backend.go b/server/services/notify/notifysubscriptions/subscriptions_backend.go index e7fb3a6d9..410c67c84 100644 --- a/server/services/notify/notifysubscriptions/subscriptions_backend.go +++ b/server/services/notify/notifysubscriptions/subscriptions_backend.go @@ -102,7 +102,7 @@ func (b *Backend) BlockChanged(evt notify.BlockChangeEvent) error { BlockType: model.TypeCard, BlockID: evt.BlockChanged.ID, SubscriberType: model.SubTypeUser, - SubscriberID: evt.ModifiedByID, + SubscriberID: evt.ModifiedBy.UserID, } if _, err = b.store.CreateSubscription(sub); err != nil { @@ -119,7 +119,7 @@ func (b *Backend) BlockChanged(evt notify.BlockChangeEvent) error { if err != nil { merr.Append(fmt.Errorf("cannot fetch subscribers for board %s: %w", evt.Board.ID, err)) } - if err = b.notifySubscribers(subs, evt.Board.ID, model.TypeBoard, evt.ModifiedByID); err != nil { + if err = b.notifySubscribers(subs, evt.Board.ID, model.TypeBoard, evt.ModifiedBy.UserID); err != nil { merr.Append(fmt.Errorf("cannot notify board subscribers for board %s: %w", evt.Board.ID, err)) } @@ -132,7 +132,7 @@ func (b *Backend) BlockChanged(evt notify.BlockChangeEvent) error { if err != nil { merr.Append(fmt.Errorf("cannot fetch subscribers for card %s: %w", evt.Card.ID, err)) } - if err = b.notifySubscribers(subs, evt.Card.ID, model.TypeCard, evt.ModifiedByID); err != nil { + if err = b.notifySubscribers(subs, evt.Card.ID, model.TypeCard, evt.ModifiedBy.UserID); err != nil { merr.Append(fmt.Errorf("cannot notify card subscribers for card %s: %w", evt.Card.ID, err)) } @@ -142,7 +142,7 @@ func (b *Backend) BlockChanged(evt notify.BlockChangeEvent) error { if err != nil { merr.Append(fmt.Errorf("cannot fetch subscribers for block %s: %w", evt.BlockChanged.ID, err)) } - if err := b.notifySubscribers(subs, evt.BlockChanged.ID, evt.BlockChanged.Type, evt.ModifiedByID); err != nil { + if err := b.notifySubscribers(subs, evt.BlockChanged.ID, evt.BlockChanged.Type, evt.ModifiedBy.UserID); err != nil { merr.Append(fmt.Errorf("cannot notify block subscribers for block %s: %w", evt.BlockChanged.ID, err)) } } @@ -183,9 +183,14 @@ func (b *Backend) OnMention(userID string, evt notify.BlockChangeEvent) { return } - // TODO: Automatically add user to board? Fail and show UI? Waiting for PM decision. - // Currently the subscription created below will only notify if the user is already - // a member of the board. + // user mentioned must be a board member to subscribe to card. + if !b.permissions.HasPermissionToBoard(userID, evt.Board.ID, model.PermissionViewBoard) { + b.logger.Debug("Not subscribing mentioned non-board member to card", + mlog.String("user_id", userID), + mlog.String("block_id", evt.BlockChanged.ID), + ) + return + } sub := &model.Subscription{ BlockType: model.TypeCard, @@ -195,7 +200,6 @@ func (b *Backend) OnMention(userID string, evt notify.BlockChangeEvent) { } var err error - if sub, err = b.store.CreateSubscription(sub); err != nil { b.logger.Warn("Cannot subscribe mentioned user to card", mlog.String("user_id", userID), diff --git a/server/services/notify/plugindelivery/mention_deliver.go b/server/services/notify/plugindelivery/mention_deliver.go index 652d3554a..6a3089f24 100644 --- a/server/services/notify/plugindelivery/mention_deliver.go +++ b/server/services/notify/plugindelivery/mention_deliver.go @@ -4,51 +4,31 @@ package plugindelivery import ( - "errors" "fmt" "github.com/mattermost/focalboard/server/services/notify" "github.com/mattermost/focalboard/server/utils" - "github.com/mattermost/mattermost-server/v6/model" + mm_model "github.com/mattermost/mattermost-server/v6/model" ) -var ( - ErrMentionPermission = errors.New("mention not permitted") -) - -// MentionDeliver notifies a user they have been mentioned in a block. -func (pd *PluginDelivery) MentionDeliver(mentionUsername string, extract string, evt notify.BlockChangeEvent) (string, error) { - user, err := userByUsername(pd.api, mentionUsername) - if err != nil { - if isErrNotFound(err) { - // not really an error; could just be someone typed "@sometext" - return "", nil - } else { - return "", fmt.Errorf("cannot lookup mentioned user: %w", err) - } - } - - // make sure mentioned user has permissions to team. - if !pd.permissions.HasPermissionToTeam(user.Id, evt.TeamID, model.PermissionViewTeam) { - return "", fmt.Errorf("mentioned user %s not member of team %s: %w", user.Id, evt.TeamID, ErrMentionPermission) - } - - author, err := pd.api.GetUserByID(evt.ModifiedByID) +// MentionDeliver notifies a user they have been mentioned in a blockv ia the plugin API. +func (pd *PluginDelivery) MentionDeliver(mentionedUser *mm_model.User, extract string, evt notify.BlockChangeEvent) (string, error) { + author, err := pd.api.GetUserByID(evt.ModifiedBy.UserID) if err != nil { return "", fmt.Errorf("cannot find user: %w", err) } - channel, err := pd.api.GetDirectChannel(user.Id, pd.botID) + channel, err := pd.api.GetDirectChannel(mentionedUser.Id, pd.botID) if err != nil { return "", fmt.Errorf("cannot get direct channel: %w", err) } link := utils.MakeCardLink(pd.serverRoot, evt.Board.TeamID, evt.Board.ID, evt.Card.ID) - post := &model.Post{ + post := &mm_model.Post{ UserId: pd.botID, ChannelId: channel.Id, Message: formatMessage(author.Username, extract, evt.Card.Title, link, evt.BlockChanged), } - return user.Id, pd.api.CreatePost(post) + return mentionedUser.Id, pd.api.CreatePost(post) } diff --git a/server/services/notify/plugindelivery/plugin_delivery.go b/server/services/notify/plugindelivery/plugin_delivery.go index 818520bae..245da9856 100644 --- a/server/services/notify/plugindelivery/plugin_delivery.go +++ b/server/services/notify/plugindelivery/plugin_delivery.go @@ -4,8 +4,6 @@ package plugindelivery import ( - "github.com/mattermost/focalboard/server/services/permissions" - mm_model "github.com/mattermost/mattermost-server/v6/model" ) @@ -39,17 +37,21 @@ type PluginAPI interface { // PluginDelivery provides ability to send notifications to direct message channels via Mattermost plugin API. type PluginDelivery struct { - botID string - serverRoot string - api PluginAPI - permissions permissions.PermissionsService + botID string + serverRoot string + api PluginAPI } -func New(botID string, serverRoot string, api PluginAPI, permissions permissions.PermissionsService) *PluginDelivery { +func New(botID string, serverRoot string, api PluginAPI) *PluginDelivery { return &PluginDelivery{ - botID: botID, - serverRoot: serverRoot, - api: api, - permissions: permissions, + botID: botID, + serverRoot: serverRoot, + api: api, } } + +// IsErrNotFound returns true if `err` or one of its wrapped children are the `ErrNotFound` +// as defined in the plugin API. +func (pd *PluginDelivery) IsErrNotFound(err error) bool { + return pd.api.IsErrNotFound(err) +} diff --git a/server/services/notify/plugindelivery/user.go b/server/services/notify/plugindelivery/user.go index 8bb71ced1..94c66ff07 100644 --- a/server/services/notify/plugindelivery/user.go +++ b/server/services/notify/plugindelivery/user.go @@ -13,14 +13,14 @@ const ( usernameSpecialChars = ".-_ " ) -func userByUsername(api PluginAPI, username string) (*mm_model.User, error) { +func (pd *PluginDelivery) UserByUsername(username string) (*mm_model.User, error) { // check for usernames that might have trailing punctuation var user *mm_model.User var err error ok := true trimmed := username for ok { - user, err = api.GetUserByUsername(trimmed) + user, err = pd.api.GetUserByUsername(trimmed) if err != nil && !isErrNotFound(err) { return nil, err } diff --git a/server/services/notify/plugindelivery/user_test.go b/server/services/notify/plugindelivery/user_test.go index 58019fe56..96d0ce8c7 100644 --- a/server/services/notify/plugindelivery/user_test.go +++ b/server/services/notify/plugindelivery/user_test.go @@ -43,7 +43,8 @@ var ( ) func Test_userByUsername(t *testing.T) { - delivery := newPlugAPIMock(mockUsers) + pluginAPI := newPlugAPIMock(mockUsers) + delivery := New("bot_id", "server_root", pluginAPI) tests := []struct { name string @@ -64,7 +65,7 @@ func Test_userByUsername(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := userByUsername(delivery, tt.uname) + got, err := delivery.UserByUsername(tt.uname) if (err != nil) != tt.wantErr { t.Errorf("userByUsername() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/server/services/notify/service.go b/server/services/notify/service.go index 4f9611427..9067cf59a 100644 --- a/server/services/notify/service.go +++ b/server/services/notify/service.go @@ -27,7 +27,7 @@ type BlockChangeEvent struct { Card *model.Block BlockChanged *model.Block BlockOld *model.Block - ModifiedByID string + ModifiedBy *model.BoardMember } type SubscriptionChangeNotifier interface { diff --git a/server/utils/callbackqueue.go b/server/utils/callbackqueue.go index 1fb4e970a..582e81876 100644 --- a/server/utils/callbackqueue.go +++ b/server/utils/callbackqueue.go @@ -2,6 +2,7 @@ package utils import ( "context" + "runtime/debug" "sync/atomic" "time" @@ -119,7 +120,11 @@ func (cn *CallbackQueue) exec(f CallbackFunc) { // don't let a panic in the callback exit the thread. defer func() { if r := recover(); r != nil { - cn.logger.Error("CallbackQueue callback panic", mlog.String("name", cn.name), mlog.Any("panic", r)) + cn.logger.Error("CallbackQueue callback panic", + mlog.String("name", cn.name), + mlog.Any("panic", r), + mlog.String("stack", string(debug.Stack())), + ) } }()