Implement permissions specs for mentions. (#2758)

* Implement permissions specs for mentions.
- public boards: admin, editor, commenter can mention team members and auto-add them to board; guests can mention board members
- private boards:  admin, editor, commenter, guest can mention board members
- viewers cannot mention

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,
+func createSubscriptionsNotifyBackend(params notifyBackendParams) (*notifysubscriptions.Backend, error) {
-	wsPluginAdapter ws.PluginAdapterInterface) (*notifysubscriptions.Backend, error) {
+	delivery, err := createDelivery(params.client, params.serverRoot)
-	//
-	delivery, err := createDelivery(params.client, params.serverRoot, params.permissions)
 	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 {

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,

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

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

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
 }

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,
+		store:       params.Store,
-		permissions: permissions,
+		permissions: params.Permissions,
-		logger:      logger,
+		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)
+}

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

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)

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.
+	// user mentioned must be a board member to subscribe to card.
-	//       Currently the subscription created below will only notify if the user is already
+	if !b.permissions.HasPermissionToBoard(userID, evt.Board.ID, model.PermissionViewBoard) {
-	//       a member of the board.
+		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),

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 (
+// MentionDeliver notifies a user they have been mentioned in a blockv ia the plugin API.
-	ErrMentionPermission = errors.New("mention not permitted")
+func (pd *PluginDelivery) MentionDeliver(mentionedUser *mm_model.User, extract string, evt notify.BlockChangeEvent) (string, error) {
-)
+	author, err := pd.api.GetUserByID(evt.ModifiedBy.UserID)
-
-// 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)
 	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)
 }

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
+	botID      string
-	serverRoot  string
+	serverRoot string
-	api         PluginAPI
+	api        PluginAPI
-	permissions permissions.PermissionsService
 }
 
-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,
+		botID:      botID,
-		serverRoot:  serverRoot,
+		serverRoot: serverRoot,
-		api:         api,
+		api:        api,
-		permissions: permissions,
 	}
 }
+
+// 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)
+}

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
 		}

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

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 {

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