From ff020d85e36ba9b51cea72db06abe42c4724a238 Mon Sep 17 00:00:00 2001 From: Doug Lauder Date: Thu, 7 Apr 2022 11:42:32 -0400 Subject: [PATCH] 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 | 21 ++++---- mattermost-plugin/server/plugin.go | 9 ++-- .../notify/notifymentions/mentions_backend.go | 17 ++++--- .../notify/notifysubscriptions/notifier.go | 35 +++++++++---- .../notify/notifysubscriptions/store.go | 2 + .../subscriptions_backend.go | 8 +++ .../notify/plugindelivery/mention_deliver.go | 16 ++++-- .../notify/plugindelivery/plugin_delivery.go | 51 ++++--------------- server/services/notify/plugindelivery/user.go | 10 +--- .../notify/plugindelivery/user_test.go | 36 +++++-------- 10 files changed, 101 insertions(+), 104 deletions(-) diff --git a/mattermost-plugin/server/notifications.go b/mattermost-plugin/server/notifications.go index 29d3ce1df..71a53e887 100644 --- a/mattermost-plugin/server/notifications.go +++ b/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" @@ -26,19 +27,20 @@ const ( ) type notifyBackendParams struct { - cfg *config.Configuration - client *pluginapi.Client - serverRoot string - logger *mlog.Logger + 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 { diff --git a/mattermost-plugin/server/plugin.go b/mattermost-plugin/server/plugin.go index 7fde51702..9d2fc2cf8 100644 --- a/mattermost-plugin/server/plugin.go +++ b/mattermost-plugin/server/plugin.go @@ -120,10 +120,11 @@ func (p *Plugin) OnActivate() error { p.wsPluginAdapter = ws.NewPluginAdapter(p.API, auth.New(cfg, db, permissionsService), db, logger) backendParams := notifyBackendParams{ - cfg: cfg, - client: client, - serverRoot: baseURL + "/boards", - logger: logger, + cfg: cfg, + client: client, + permissions: permissionsService, + serverRoot: baseURL + "/boards", + logger: logger, } var notifyBackends []notify.Backend diff --git a/server/services/notify/notifymentions/mentions_backend.go b/server/services/notify/notifymentions/mentions_backend.go index 196acd6d3..2249c5c15 100644 --- a/server/services/notify/notifymentions/mentions_backend.go +++ b/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" @@ -24,17 +25,19 @@ type MentionListener interface { // Backend provides the notification backend for @mentions. type Backend struct { - delivery MentionDelivery - logger *mlog.Logger + 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, - logger: logger, + 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 } diff --git a/server/services/notify/notifysubscriptions/notifier.go b/server/services/notify/notifysubscriptions/notifier.go index 098aa7911..c39b0a450 100644 --- a/server/services/notify/notifysubscriptions/notifier.go +++ b/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" @@ -31,10 +32,11 @@ var ( // via notifications hints written to the database so that fewer notifications are sent for active // blocks. type notifier struct { - serverRoot string - store Store - delivery SubscriptionDelivery - logger *mlog.Logger + serverRoot string + store Store + permissions permissions.PermissionsService + delivery SubscriptionDelivery + logger *mlog.Logger hints chan *model.NotificationHint @@ -44,12 +46,13 @@ type notifier struct { func newNotifier(params BackendParams) *notifier { return ¬ifier{ - serverRoot: params.ServerRoot, - store: params.Store, - delivery: params.Delivery, - logger: params.Logger, - done: nil, - hints: make(chan *model.NotificationHint, hintQueueSize), + serverRoot: params.ServerRoot, + store: params.Store, + permissions: params.Permissions, + delivery: params.Delivery, + logger: params.Logger, + done: nil, + hints: make(chan *model.NotificationHint, hintQueueSize), } } @@ -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), diff --git a/server/services/notify/notifysubscriptions/store.go b/server/services/notify/notifysubscriptions/store.go index bac406cc7..4de0f1e9f 100644 --- a/server/services/notify/notifysubscriptions/store.go +++ b/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) diff --git a/server/services/notify/notifysubscriptions/subscriptions_backend.go b/server/services/notify/notifysubscriptions/subscriptions_backend.go index 4b543ba7c..e7fb3a6d9 100644 --- a/server/services/notify/notifysubscriptions/subscriptions_backend.go +++ b/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, diff --git a/server/services/notify/plugindelivery/mention_deliver.go b/server/services/notify/plugindelivery/mention_deliver.go index 325443dd2..652d3554a 100644 --- a/server/services/notify/plugindelivery/mention_deliver.go +++ b/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) } diff --git a/server/services/notify/plugindelivery/plugin_delivery.go b/server/services/notify/plugindelivery/plugin_delivery.go index 764b9095d..818520bae 100644 --- a/server/services/notify/plugindelivery/plugin_delivery.go +++ b/server/services/notify/plugindelivery/plugin_delivery.go @@ -4,10 +4,7 @@ package plugindelivery import ( - "fmt" - - "github.com/mattermost/focalboard/server/services/notify" - "github.com/mattermost/focalboard/server/utils" + "github.com/mattermost/focalboard/server/services/permissions" mm_model "github.com/mattermost/mattermost-server/v6/model" ) @@ -42,45 +39,17 @@ 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 + 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, + 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) -} diff --git a/server/services/notify/plugindelivery/user.go b/server/services/notify/plugindelivery/user.go index f9efec030..8bb71ced1 100644 --- a/server/services/notify/plugindelivery/user.go +++ b/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. - member, err := api.GetTeamMember(teamID, user.Id) - if err != nil { - return nil, err - } - - return member, nil + return user, nil } // trimUsernameSpecialChar tries to remove the last character from word if it diff --git a/server/services/notify/plugindelivery/user_test.go b/server/services/notify/plugindelivery/user_test.go index 6be9f5194..58019fe56 100644 --- a/server/services/notify/plugindelivery/user_test.go +++ b/server/services/notify/plugindelivery/user_test.go @@ -42,43 +42,35 @@ var ( } ) -func userToMember(user *mm_model.User, teamID string) *mm_model.TeamMember { - return &mm_model.TeamMember{ - TeamId: teamID, - UserId: user.Id, - } -} - -func Test_teamMemberFromUsername(t *testing.T) { +func Test_userByUsername(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 with period", uname: user1.Username + ".", teamID: defTeamID, want: userToMember(user1, defTeamID), wantErr: false}, - {name: "user1 with period plus more", uname: user1.Username + ". ", teamID: defTeamID, want: userToMember(user1, defTeamID), wantErr: false}, - {name: "user2 with periods", uname: user2.Username + "...", teamID: defTeamID, want: userToMember(user2, defTeamID), wantErr: false}, - {name: "user2 with underscore", uname: user2.Username + "_", teamID: defTeamID, want: userToMember(user2, defTeamID), wantErr: false}, - {name: "user2 with hyphen plus more", uname: user2.Username + "- ", teamID: defTeamID, want: userToMember(user2, defTeamID), wantErr: false}, - {name: "user2 with hyphen plus all", uname: user2.Username + ".-_ ", teamID: defTeamID, want: userToMember(user2, defTeamID), wantErr: false}, - {name: "user3 with underscore", uname: user3.Username + "_", teamID: defTeamID, want: userToMember(user3, defTeamID), wantErr: false}, - {name: "user4 missing", uname: user4.Username, want: nil, teamID: defTeamID, wantErr: true}, - {name: "user5 wrong team", uname: user5.Username, teamID: "bogus_team", want: nil, wantErr: true}, + {name: "user1", uname: user1.Username, want: user1, wantErr: false}, + {name: "user1 with period", uname: user1.Username + ".", want: user1, wantErr: false}, + {name: "user1 with period plus more", uname: user1.Username + ". ", want: user1, wantErr: false}, + {name: "user2 with periods", uname: user2.Username + "...", want: user2, wantErr: false}, + {name: "user2 with underscore", uname: user2.Username + "_", want: user2, wantErr: false}, + {name: "user2 with hyphen plus more", uname: user2.Username + "- ", want: user2, wantErr: false}, + {name: "user2 with hyphen plus all", uname: user2.Username + ".-_ ", want: user2, wantErr: false}, + {name: "user3 with underscore", uname: user3.Username + "_", want: user3, wantErr: false}, + {name: "user4 missing", uname: user4.Username, 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) } }) }