Don't notify non-board members of card changes (#2718)

* Don't notify non-board members of card changes
- include permissions service in notifications backends
- use permissions service to ensure @mentions are to users on team
- use permissions service to ensure subscribers are members of board

mattermost-plugin/server/notifications.go

@@ -8,6 +8,7 @@ import (
 	"github.com/mattermost/focalboard/server/services/notify/notifymentions"
 	"github.com/mattermost/focalboard/server/services/notify/notifysubscriptions"
 	"github.com/mattermost/focalboard/server/services/notify/plugindelivery"
+	"github.com/mattermost/focalboard/server/services/permissions"
 	"github.com/mattermost/focalboard/server/services/store"
 	"github.com/mattermost/focalboard/server/ws"
 
@@ -28,17 +29,18 @@ const (
 type notifyBackendParams struct {
 	cfg         *config.Configuration
 	client      *pluginapi.Client
+	permissions permissions.PermissionsService
 	serverRoot  string
 	logger      *mlog.Logger
 }
 
 func createMentionsNotifyBackend(params notifyBackendParams) (*notifymentions.Backend, error) {
-	delivery, err := createDelivery(params.client, params.serverRoot)
+	delivery, err := createDelivery(params.client, params.serverRoot, params.permissions)
 	if err != nil {
 		return nil, err
 	}
 
-	backend := notifymentions.New(delivery, params.logger)
+	backend := notifymentions.New(delivery, params.permissions, params.logger)
 
 	return backend, nil
 }
@@ -46,7 +48,7 @@ func createMentionsNotifyBackend(params notifyBackendParams) (*notifymentions.Ba
 func createSubscriptionsNotifyBackend(params notifyBackendParams, store store.Store,
 	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
 	}
@@ -54,6 +56,7 @@ func createSubscriptionsNotifyBackend(params notifyBackendParams, store store.St
 	backendParams := notifysubscriptions.BackendParams{
 		ServerRoot:             params.serverRoot,
 		Store:                  store,
+		Permissions:            params.permissions,
 		Delivery:               delivery,
 		WSAdapter:              wsPluginAdapter,
 		Logger:                 params.logger,
@@ -65,7 +68,7 @@ func createSubscriptionsNotifyBackend(params notifyBackendParams, store store.St
 	return backend, nil
 }
 
-func createDelivery(client *pluginapi.Client, serverRoot string) (*plugindelivery.PluginDelivery, error) {
+func createDelivery(client *pluginapi.Client, serverRoot string, permissions permissions.PermissionsService) (*plugindelivery.PluginDelivery, error) {
 	bot := &model.Bot{
 		Username:    botUsername,
 		DisplayName: botDisplayname,
@@ -78,7 +81,7 @@ func createDelivery(client *pluginapi.Client, serverRoot string) (*plugindeliver
 
 	pluginAPI := &pluginAPIAdapter{client: client}
 
-	return plugindelivery.New(botID, serverRoot, pluginAPI), nil
+	return plugindelivery.New(botID, serverRoot, pluginAPI, permissions), nil
 }
 
 type pluginAPIAdapter struct {

mattermost-plugin/server/plugin.go

@@ -122,6 +122,7 @@ func (p *Plugin) OnActivate() error {
 	backendParams := notifyBackendParams{
 		cfg:         cfg,
 		client:      client,
+		permissions: permissionsService,
 		serverRoot:  baseURL + "/boards",
 		logger:      logger,
 	}

server/services/notify/notifymentions/mentions_backend.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/mattermost/focalboard/server/model"
 	"github.com/mattermost/focalboard/server/services/notify"
+	"github.com/mattermost/focalboard/server/services/permissions"
 	"github.com/wiggin77/merror"
 
 	"github.com/mattermost/mattermost-server/v6/shared/mlog"
@@ -25,15 +26,17 @@ type MentionListener interface {
 // Backend provides the notification backend for @mentions.
 type Backend struct {
 	delivery    MentionDelivery
+	permissions permissions.PermissionsService
 	logger      *mlog.Logger
 
 	mux       sync.RWMutex
 	listeners []MentionListener
 }
 
-func New(delivery MentionDelivery, logger *mlog.Logger) *Backend {
+func New(delivery MentionDelivery, permissions permissions.PermissionsService, logger *mlog.Logger) *Backend {
 	return &Backend{
 		delivery:    delivery,
+		permissions: permissions,
 		logger:      logger,
 	}
 }
@@ -80,7 +83,9 @@ func (b *Backend) BlockChanged(evt notify.BlockChangeEvent) error {
 		return nil
 	}
 
-	if evt.BlockChanged.Type != model.TypeText && evt.BlockChanged.Type != model.TypeComment {
+	switch evt.BlockChanged.Type {
+	case model.TypeText, model.TypeComment, model.TypeImage:
+	default:
 		return nil
 	}
 

server/services/notify/notifysubscriptions/notifier.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/mattermost/focalboard/server/model"
+	"github.com/mattermost/focalboard/server/services/permissions"
 	"github.com/mattermost/focalboard/server/services/store"
 	"github.com/mattermost/focalboard/server/utils"
 	"github.com/wiggin77/merror"
@@ -33,6 +34,7 @@ var (
 type notifier struct {
 	serverRoot  string
 	store       Store
+	permissions permissions.PermissionsService
 	delivery    SubscriptionDelivery
 	logger      *mlog.Logger
 
@@ -46,6 +48,7 @@ func newNotifier(params BackendParams) *notifier {
 	return &notifier{
 		serverRoot:  params.ServerRoot,
 		store:       params.Store,
+		permissions: params.Permissions,
 		delivery:    params.Delivery,
 		logger:      params.Logger,
 		done:        nil,
@@ -216,7 +219,7 @@ func (n *notifier) notifySubscribers(hint *model.NotificationHint) error {
 			// don't notify the author of their own changes.
 			authorName, isAuthor := diffAuthors[sub.SubscriberID]
 			if isAuthor && len(diffAuthors) == 1 {
-				n.logger.Debug("notifySubscribers - deliver, skipping author",
+				n.logger.Debug("notifySubscribers - skipping author",
 					mlog.Any("hint", hint),
 					mlog.String("author_id", sub.SubscriberID),
 					mlog.String("author_username", authorName),
@@ -224,6 +227,16 @@ func (n *notifier) notifySubscribers(hint *model.NotificationHint) error {
 				continue
 			}
 
+			// make sure the subscriber still has permissions for the board.
+			if !n.permissions.HasPermissionToBoard(sub.SubscriberID, board.ID, model.PermissionViewBoard) {
+				n.logger.Debug("notifySubscribers - skipping non-board member",
+					mlog.Any("hint", hint),
+					mlog.String("subscriber_id", sub.SubscriberID),
+					mlog.String("board_id", board.ID),
+				)
+				continue
+			}
+
 			n.logger.Debug("notifySubscribers - deliver",
 				mlog.Any("hint", hint),
 				mlog.String("modified_by_id", hint.ModifiedByID),

server/services/notify/notifysubscriptions/store.go

@@ -17,6 +17,8 @@ type Store interface {
 
 	GetUserByID(userID string) (*model.User, error)
 
+	GetMemberForBoard(boardID, userID string) (*model.BoardMember, error)
+
 	CreateSubscription(sub *model.Subscription) (*model.Subscription, error)
 	GetSubscribersForBlock(blockID string) ([]*model.Subscriber, error)
 	GetSubscribersCountForBlock(blockID string) (int, error)

server/services/notify/notifysubscriptions/subscriptions_backend.go

@@ -9,6 +9,7 @@ import (
 
 	"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"
 
@@ -22,6 +23,7 @@ const (
 type BackendParams struct {
 	ServerRoot             string
 	Store                  Store
+	Permissions            permissions.PermissionsService
 	Delivery               SubscriptionDelivery
 	WSAdapter              ws.Adapter
 	Logger                 *mlog.Logger
@@ -32,6 +34,7 @@ type BackendParams struct {
 // Backend provides the notification backend for subscriptions.
 type Backend struct {
 	store                  Store
+	permissions            permissions.PermissionsService
 	delivery               SubscriptionDelivery
 	notifier               *notifier
 	wsAdapter              ws.Adapter
@@ -44,6 +47,7 @@ func New(params BackendParams) *Backend {
 	return &Backend{
 		store:                  params.Store,
 		delivery:               params.Delivery,
+		permissions:            params.Permissions,
 		notifier:               newNotifier(params),
 		wsAdapter:              params.WSAdapter,
 		logger:                 params.Logger,
@@ -179,6 +183,10 @@ 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.
+
 	sub := &model.Subscription{
 		BlockType:      model.TypeCard,
 		BlockID:        evt.Card.ID,

server/services/notify/plugindelivery/mention_deliver.go

@@ -4,6 +4,7 @@
 package plugindelivery
 
 import (
+	"errors"
 	"fmt"
 
 	"github.com/mattermost/focalboard/server/services/notify"
@@ -12,9 +13,13 @@ import (
 	"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) {
-	member, err := teamMemberFromUsername(pd.api, mentionUsername, evt.TeamID)
+	user, err := userByUsername(pd.api, mentionUsername)
 	if err != nil {
 		if isErrNotFound(err) {
 			// not really an error; could just be someone typed "@sometext"
@@ -24,12 +29,17 @@ func (pd *PluginDelivery) MentionDeliver(mentionUsername string, extract string,
 		}
 	}
 
+	// 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(member.UserId, pd.botID)
+	channel, err := pd.api.GetDirectChannel(user.Id, pd.botID)
 	if err != nil {
 		return "", fmt.Errorf("cannot get direct channel: %w", err)
 	}
@@ -40,5 +50,5 @@ func (pd *PluginDelivery) MentionDeliver(mentionUsername string, extract string,
 		ChannelId: channel.Id,
 		Message:   formatMessage(author.Username, extract, evt.Card.Title, link, evt.BlockChanged),
 	}
-	return member.UserId, pd.api.CreatePost(post)
+	return user.Id, pd.api.CreatePost(post)
 }

server/services/notify/plugindelivery/plugin_delivery.go

@@ -4,10 +4,7 @@
 package plugindelivery
 
 import (
-	"fmt"
+	"github.com/mattermost/focalboard/server/services/permissions"
-
-	"github.com/mattermost/focalboard/server/services/notify"
-	"github.com/mattermost/focalboard/server/utils"
 
 	mm_model "github.com/mattermost/mattermost-server/v6/model"
 )
@@ -45,42 +42,14 @@ type PluginDelivery struct {
 	botID       string
 	serverRoot  string
 	api         PluginAPI
+	permissions permissions.PermissionsService
 }
 
-func New(botID string, serverRoot string, api PluginAPI) *PluginDelivery {
+func New(botID string, serverRoot string, api PluginAPI, permissions permissions.PermissionsService) *PluginDelivery {
 	return &PluginDelivery{
 		botID:       botID,
 		serverRoot:  serverRoot,
 		api:         api,
+		permissions: permissions,
 	}
 }
-
-func (pd *PluginDelivery) Deliver(mentionUsername string, extract string, evt notify.BlockChangeEvent) error {
-	member, err := teamMemberFromUsername(pd.api, mentionUsername, evt.TeamID)
-	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)
-		}
-	}
-
-	author, err := pd.api.GetUserByID(evt.ModifiedByID)
-	if err != nil {
-		return fmt.Errorf("cannot find user: %w", err)
-	}
-
-	channel, err := pd.api.GetDirectChannel(member.UserId, pd.botID)
-	if err != nil {
-		return fmt.Errorf("cannot get direct channel: %w", err)
-	}
-	link := utils.MakeCardLink(pd.serverRoot, evt.TeamID, evt.Board.ID, evt.Card.ID)
-
-	post := &mm_model.Post{
-		UserId:    pd.botID,
-		ChannelId: channel.Id,
-		Message:   formatMessage(author.Username, extract, evt.Card.Title, link, evt.BlockChanged),
-	}
-	return pd.api.CreatePost(post)
-}

server/services/notify/plugindelivery/user.go

@@ -13,7 +13,7 @@ const (
 	usernameSpecialChars = ".-_ "
 )
 
-func teamMemberFromUsername(api PluginAPI, username string, teamID string) (*mm_model.TeamMember, error) {
+func userByUsername(api PluginAPI, username string) (*mm_model.User, error) {
 	// check for usernames that might have trailing punctuation
 	var user *mm_model.User
 	var err error
@@ -36,13 +36,7 @@ func teamMemberFromUsername(api PluginAPI, username string, teamID string) (*mm_
 		return nil, err
 	}
 
-	// make sure user is member of team.
+	return user, nil
-	member, err := api.GetTeamMember(teamID, user.Id)
-	if err != nil {
-		return nil, err
-	}
-
-	return member, nil
 }
 
 // trimUsernameSpecialChar tries to remove the last character from word if it

server/services/notify/plugindelivery/user_test.go

@@ -42,43 +42,35 @@ var (
 	}
 )
 
-func userToMember(user *mm_model.User, teamID string) *mm_model.TeamMember {
+func Test_userByUsername(t *testing.T) {
-	return &mm_model.TeamMember{
-		TeamId: teamID,
-		UserId: user.Id,
-	}
-}
-
-func Test_teamMemberFromUsername(t *testing.T) {
 	delivery := newPlugAPIMock(mockUsers)
 
 	tests := []struct {
 		name    string
 		uname   string
 		teamID  string
-		want    *mm_model.TeamMember
+		want    *mm_model.User
 		wantErr bool
 	}{
-		{name: "user1", uname: user1.Username, teamID: defTeamID, want: userToMember(user1, defTeamID), wantErr: false},
+		{name: "user1", uname: user1.Username, want: user1, wantErr: false},
-		{name: "user1 with period", uname: user1.Username + ".", teamID: defTeamID, want: userToMember(user1, defTeamID), wantErr: false},
+		{name: "user1 with period", uname: user1.Username + ".", want: user1, wantErr: false},
-		{name: "user1 with period plus more", uname: user1.Username + ". ", teamID: defTeamID, want: userToMember(user1, defTeamID), wantErr: false},
+		{name: "user1 with period plus more", uname: user1.Username + ". ", want: user1, wantErr: false},
-		{name: "user2 with periods", uname: user2.Username + "...", teamID: defTeamID, want: userToMember(user2, defTeamID), wantErr: false},
+		{name: "user2 with periods", uname: user2.Username + "...", want: user2, wantErr: false},
-		{name: "user2 with underscore", uname: user2.Username + "_", teamID: defTeamID, want: userToMember(user2, defTeamID), wantErr: false},
+		{name: "user2 with underscore", uname: user2.Username + "_", want: user2, wantErr: false},
-		{name: "user2 with hyphen plus more", uname: user2.Username + "- ", teamID: defTeamID, want: userToMember(user2, defTeamID), wantErr: false},
+		{name: "user2 with hyphen plus more", uname: user2.Username + "- ", want: user2, wantErr: false},
-		{name: "user2 with hyphen plus all", uname: user2.Username + ".-_ ", teamID: defTeamID, want: userToMember(user2, defTeamID), wantErr: false},
+		{name: "user2 with hyphen plus all", uname: user2.Username + ".-_ ", want: user2, wantErr: false},
-		{name: "user3 with underscore", uname: user3.Username + "_", teamID: defTeamID, want: userToMember(user3, defTeamID), wantErr: false},
+		{name: "user3 with underscore", uname: user3.Username + "_", want: user3, wantErr: false},
-		{name: "user4 missing", uname: user4.Username, want: nil, teamID: defTeamID, wantErr: true},
+		{name: "user4 missing", uname: user4.Username, want: nil, wantErr: true},
-		{name: "user5 wrong team", uname: user5.Username, teamID: "bogus_team", want: nil, wantErr: true},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := teamMemberFromUsername(delivery, tt.uname, tt.teamID)
+			got, err := userByUsername(delivery, tt.uname)
 			if (err != nil) != tt.wantErr {
-				t.Errorf("userFromUsername() error = %v, wantErr %v", err, tt.wantErr)
+				t.Errorf("userByUsername() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
 			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("userFromUsername()\ngot:\n%v\nwant:\n%v\n", got, tt.want)
+				t.Errorf("userByUsername()\ngot:\n%v\nwant:\n%v\n", got, tt.want)
 			}
 		})
 	}