Improves notify props validation (#24031)

* Adds the channel member notify props max runes restriction

* Fix translations
This commit is contained in:
Miguel de la Cruz 2023-07-18 17:25:11 +02:00 committed by GitHub
parent 3c31629813
commit 4803889158
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 97 additions and 16 deletions

View File

@ -1338,14 +1338,17 @@ func (a *App) UpdateChannelMemberNotifyProps(c request.CTX, data map[string]stri
member, err := a.Srv().Store().Channel().UpdateMemberNotifyProps(channelID, userID, filteredProps) member, err := a.Srv().Store().Channel().UpdateMemberNotifyProps(channelID, userID, filteredProps)
if err != nil { if err != nil {
var appErr *model.AppError var appErr *model.AppError
var invErr *store.ErrInvalidInput
var nfErr *store.ErrNotFound var nfErr *store.ErrNotFound
switch { switch {
case errors.As(err, &appErr): case errors.As(err, &appErr):
return nil, appErr return nil, appErr
case errors.As(err, &invErr):
return nil, model.NewAppError("updateMemberNotifyProps", "app.channel.update_member.notify_props_limit_exceeded.app_error", nil, "", http.StatusBadRequest).Wrap(err)
case errors.As(err, &nfErr): case errors.As(err, &nfErr):
return nil, model.NewAppError("updateMemberNotifyProps", MissingChannelMemberError, nil, "", http.StatusNotFound).Wrap(err) return nil, model.NewAppError("updateMemberNotifyProps", MissingChannelMemberError, nil, "", http.StatusNotFound).Wrap(err)
default: default:
return nil, model.NewAppError("updateMemberNotifyProps", "app.channel.get_member.app_error", nil, "", http.StatusInternalServerError).Wrap(err) return nil, model.NewAppError("updateMemberNotifyProps", "app.channel.update_member.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
} }
} }

View File

@ -390,7 +390,6 @@ func (a *App) buildUserChannelMemberships(userID string, teamID string) (*[]impo
} }
func (a *App) buildUserNotifyProps(notifyProps model.StringMap) *imports.UserNotifyPropsImportData { func (a *App) buildUserNotifyProps(notifyProps model.StringMap) *imports.UserNotifyPropsImportData {
getProp := func(key string) *string { getProp := func(key string) *string {
if v, ok := notifyProps[key]; ok { if v, ok := notifyProps[key]; ok {
return &v return &v

View File

@ -11,6 +11,7 @@ import (
"strconv" "strconv"
"strings" "strings"
"time" "time"
"unicode/utf8"
sq "github.com/mattermost/squirrel" sq "github.com/mattermost/squirrel"
"github.com/pkg/errors" "github.com/pkg/errors"
@ -1916,10 +1917,15 @@ func (s SqlChannelStore) UpdateMemberNotifyProps(channelID, userID string, props
} }
defer finalizeTransactionX(tx, &err) defer finalizeTransactionX(tx, &err)
jsonNotifyProps := model.MapToJSON(props)
if utf8.RuneCountInString(jsonNotifyProps) > model.ChannelMemberNotifyPropsMaxRunes {
return nil, store.NewErrInvalidInput("ChannelMember", "NotifyProps", props)
}
if s.DriverName() == model.DatabaseDriverPostgres { if s.DriverName() == model.DatabaseDriverPostgres {
sql, args, err2 := s.getQueryBuilder(). sql, args, err2 := s.getQueryBuilder().
Update("channelmembers"). Update("channelmembers").
Set("notifyprops", sq.Expr("notifyprops || ?::jsonb", model.MapToJSON(props))). Set("notifyprops", sq.Expr("notifyprops || ?::jsonb", jsonNotifyProps)).
Where(sq.Eq{ Where(sq.Eq{
"userid": userID, "userid": userID,
"channelid": channelID, "channelid": channelID,

View File

@ -1082,6 +1082,15 @@ func testChannelSaveMember(t *testing.T, ss store.Store) {
require.IsType(t, &store.ErrConflict{}, nErr) require.IsType(t, &store.ErrConflict{}, nErr)
}) })
t.Run("should fail if notify props are too big", func(t *testing.T) {
channelID := model.NewId()
props := model.GetDefaultChannelNotifyProps()
props["property"] = strings.Repeat("Z", model.ChannelMemberNotifyPropsMaxRunes)
member := &model.ChannelMember{ChannelId: channelID, UserId: u1.Id, NotifyProps: props}
_, nErr := ss.Channel().SaveMember(member)
require.ErrorContains(t, nErr, "channel_member.is_valid.notify_props")
})
t.Run("insert member correctly (in channel without channel scheme and team without scheme)", func(t *testing.T) { t.Run("insert member correctly (in channel without channel scheme and team without scheme)", func(t *testing.T) {
team := &model.Team{ team := &model.Team{
DisplayName: "Name", DisplayName: "Name",
@ -1581,6 +1590,15 @@ func testChannelSaveMultipleMembers(t *testing.T, ss store.Store) {
require.IsType(t, &store.ErrConflict{}, nErr) require.IsType(t, &store.ErrConflict{}, nErr)
}) })
t.Run("should fail if notify props are too big", func(t *testing.T) {
channelID := model.NewId()
props := model.GetDefaultChannelNotifyProps()
props["property"] = strings.Repeat("Z", model.ChannelMemberNotifyPropsMaxRunes)
member := &model.ChannelMember{ChannelId: channelID, UserId: u1.Id, NotifyProps: props}
_, nErr := ss.Channel().SaveMultipleMembers([]*model.ChannelMember{member})
require.ErrorContains(t, nErr, "channel_member.is_valid.notify_props")
})
t.Run("insert members correctly (in channel without channel scheme and team without scheme)", func(t *testing.T) { t.Run("insert members correctly (in channel without channel scheme and team without scheme)", func(t *testing.T) {
team := &model.Team{ team := &model.Team{
DisplayName: "Name", DisplayName: "Name",
@ -2109,6 +2127,18 @@ func testChannelUpdateMember(t *testing.T, ss store.Store) {
require.Equal(t, "model.channel_member.is_valid.channel_id.app_error", appErr.Id) require.Equal(t, "model.channel_member.is_valid.channel_id.app_error", appErr.Id)
}) })
t.Run("should fail with invalid error if notify props are too big", func(t *testing.T) {
props := model.GetDefaultChannelNotifyProps()
props["property"] = strings.Repeat("Z", model.ChannelMemberNotifyPropsMaxRunes)
member := &model.ChannelMember{ChannelId: model.NewId(), UserId: u1.Id, NotifyProps: props}
_, nErr := ss.Channel().UpdateMember(member)
require.Error(t, nErr)
var appErr *model.AppError
require.ErrorAs(t, nErr, &appErr)
require.Equal(t, "model.channel_member.is_valid.notify_props.app_error", appErr.Id)
})
t.Run("insert member correctly (in channel without channel scheme and team without scheme)", func(t *testing.T) { t.Run("insert member correctly (in channel without channel scheme and team without scheme)", func(t *testing.T) {
team := &model.Team{ team := &model.Team{
DisplayName: "Name", DisplayName: "Name",
@ -2614,6 +2644,18 @@ func testChannelUpdateMultipleMembers(t *testing.T, ss store.Store) {
require.IsType(t, &store.ErrConflict{}, nErr) require.IsType(t, &store.ErrConflict{}, nErr)
}) })
t.Run("should fail with invalid error if notify props are too big", func(t *testing.T) {
props := model.GetDefaultChannelNotifyProps()
props["property"] = strings.Repeat("Z", model.ChannelMemberNotifyPropsMaxRunes)
member := &model.ChannelMember{ChannelId: model.NewId(), UserId: u1.Id, NotifyProps: props}
_, nErr := ss.Channel().SaveMultipleMembers([]*model.ChannelMember{member})
require.Error(t, nErr)
var appErr *model.AppError
require.ErrorAs(t, nErr, &appErr)
require.Equal(t, "model.channel_member.is_valid.notify_props.app_error", appErr.Id)
})
t.Run("insert members correctly (in channel without channel scheme and team without scheme)", func(t *testing.T) { t.Run("insert members correctly (in channel without channel scheme and team without scheme)", func(t *testing.T) {
team := &model.Team{ team := &model.Team{
DisplayName: "Name", DisplayName: "Name",
@ -3152,6 +3194,14 @@ func testChannelUpdateMemberNotifyProps(t *testing.T, ss store.Store) {
require.NoError(t, nErr) require.NoError(t, nErr)
// Verify props. // Verify props.
assert.Equal(t, props, member.NotifyProps) assert.Equal(t, props, member.NotifyProps)
t.Run("should fail with invalid input if the notify props are too big", func(t *testing.T) {
props["property"] = strings.Repeat("Z", model.ChannelMemberNotifyPropsMaxRunes)
member, err = ss.Channel().UpdateMemberNotifyProps(member.ChannelId, member.UserId, props)
var invErr *store.ErrInvalidInput
require.ErrorAs(t, err, &invErr)
require.Nil(t, member)
})
} }
func testChannelRemoveMember(t *testing.T, ss store.Store) { func testChannelRemoveMember(t *testing.T, ss store.Store) {

View File

@ -4887,6 +4887,14 @@
"id": "app.channel.update_last_viewed_at_post.app_error", "id": "app.channel.update_last_viewed_at_post.app_error",
"translation": "Unable to mark channel as unread." "translation": "Unable to mark channel as unread."
}, },
{
"id": "app.channel.update_member.app_error",
"translation": "Unable to update the channel member."
},
{
"id": "app.channel.update_member.notify_props_limit_exceeded.app_error",
"translation": "Unable to update the channel member, notify props size limit exceeded."
},
{ {
"id": "app.channel.user_belongs_to_channels.app_error", "id": "app.channel.user_belongs_to_channels.app_error",
"translation": "Unable to determine if the user belongs to a list of channels." "translation": "Unable to determine if the user belongs to a list of channels."
@ -8223,6 +8231,10 @@
"id": "model.channel_member.is_valid.notify_level.app_error", "id": "model.channel_member.is_valid.notify_level.app_error",
"translation": "Invalid notify level." "translation": "Invalid notify level."
}, },
{
"id": "model.channel_member.is_valid.notify_props.app_error",
"translation": "Notify props size limit exceeded."
},
{ {
"id": "model.channel_member.is_valid.push_level.app_error", "id": "model.channel_member.is_valid.push_level.app_error",
"translation": "Invalid push notification level." "translation": "Invalid push notification level."

View File

@ -6,22 +6,24 @@ package model
import ( import (
"net/http" "net/http"
"strings" "strings"
"unicode/utf8"
) )
const ( const (
ChannelNotifyDefault = "default" ChannelNotifyDefault = "default"
ChannelNotifyAll = "all" ChannelNotifyAll = "all"
ChannelNotifyMention = "mention" ChannelNotifyMention = "mention"
ChannelNotifyNone = "none" ChannelNotifyNone = "none"
ChannelMarkUnreadAll = "all" ChannelMarkUnreadAll = "all"
ChannelMarkUnreadMention = "mention" ChannelMarkUnreadMention = "mention"
IgnoreChannelMentionsDefault = "default" IgnoreChannelMentionsDefault = "default"
IgnoreChannelMentionsOff = "off" IgnoreChannelMentionsOff = "off"
IgnoreChannelMentionsOn = "on" IgnoreChannelMentionsOn = "on"
IgnoreChannelMentionsNotifyProp = "ignore_channel_mentions" IgnoreChannelMentionsNotifyProp = "ignore_channel_mentions"
ChannelAutoFollowThreadsOff = "off" ChannelAutoFollowThreadsOff = "off"
ChannelAutoFollowThreadsOn = "on" ChannelAutoFollowThreadsOn = "on"
ChannelAutoFollowThreads = "channel_auto_follow_threads" ChannelAutoFollowThreads = "channel_auto_follow_threads"
ChannelMemberNotifyPropsMaxRunes = 800000
) )
type ChannelUnread struct { type ChannelUnread struct {
@ -186,6 +188,11 @@ func (o *ChannelMember) IsValid() *AppError {
map[string]any{"Limit": UserRolesMaxLength}, "", http.StatusBadRequest) map[string]any{"Limit": UserRolesMaxLength}, "", http.StatusBadRequest)
} }
jsonStringNotifyProps := string(ToJSON(o.NotifyProps))
if utf8.RuneCountInString(jsonStringNotifyProps) > ChannelMemberNotifyPropsMaxRunes {
return NewAppError("ChannelMember.IsValid", "model.channel_member.is_valid.notify_props.app_error", nil, "channel_id="+o.ChannelId+" user_id="+o.UserId, http.StatusBadRequest)
}
return nil return nil
} }

View File

@ -4,6 +4,7 @@
package model package model
import ( import (
"strings"
"testing" "testing"
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
@ -37,4 +38,7 @@ func TestChannelMemberIsValid(t *testing.T) {
o.Roles = "" o.Roles = ""
require.Nil(t, o.IsValid(), "should be invalid") require.Nil(t, o.IsValid(), "should be invalid")
o.NotifyProps["property"] = strings.Repeat("Z", ChannelMemberNotifyPropsMaxRunes)
require.NotNil(t, o.IsValid(), "should be invalid")
} }