Skip to content

Commit

Permalink
update for adding multiple members (mattermost#25128)
Browse files Browse the repository at this point in the history 
* update for adding multiple members

* fix unit test

* more test fixes

* add another unit test

* fix object passed by client4

* revert package-lock.json

* revert package-lock.json

* add length check

* limit size of lists in API requests

* revert package-lock

* add batching to front end

* add batching to front end

* fix bad merge

* update return type

* remove unnecessary permisssion check, add unit test

* fixes and add tests from review

* revert changes adding limits to other apis

* fixes

* clean-up from code review

* fix unit test call

* revert back to interface{}, fix unit test

---------

Co-authored-by: Mattermost Build <[email protected]>
  • Loading branch information
@sbishel @mattermost-build
sbishel and mattermost-build authored Jun 25, 2024
1 parent 4f68dbb commit 6fd8949
Show file tree
Hide file tree
Showing 13 changed files with 405 additions and 100 deletions.
23 changes: 15 additions & 8 deletions api/v4/source/channels.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,9 @@
type: array
items:
type: string
description: User ids to be in the group message channel
minItems: 3
maxItems: 8
description: User ids to be in the group message channel.
required: true
responses:
"201":
Expand Down Expand Up @@ -408,7 +410,7 @@
type: array
items:
type: string
description: List of channel ids
description: List of channel ids.
required: true
responses:
"200":
Expand Down Expand Up @@ -1329,8 +1331,8 @@
post:
tags:
- channels
summary: Add user to channel
description: Add a user to a channel by creating a channel member object.
summary: Add user(s) to channel
description: Add a user(s) to a channel by creating a channel member object(s).
operationId: AddChannelMember
parameters:
- name: channel_id
Expand All @@ -1344,12 +1346,17 @@
application/json:
schema:
type: object
required:
- user_id
properties:
user_id:
type: string
description: The ID of user to add into the channel
description: The ID of user to add into the channel, for backwards compatibility.
user_ids:
type: array
items:
type: string
minItems: 1
maxItems: 1000
description: The IDs of users to add into the channel, required if 'user_id' doess not exist.
post_root_id:
type: string
description: The ID of root post where link to add channel member
Expand Down Expand Up @@ -1392,7 +1399,7 @@
type: array
items:
type: string
description: List of user ids
description: List of user ids.
required: true
responses:
"200":
Expand Down
204 changes: 125 additions & 79 deletions server/channels/api4/channel.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (
"github.com/mattermost/mattermost/server/v8/channels/audit"
)

const maxListSize = 1000

func (api *API) InitChannel() {
api.BaseRoutes.Channels.Handle("", api.APISessionRequired(getAllChannels)).Methods("GET")
api.BaseRoutes.Channels.Handle("", api.APISessionRequired(createChannel)).Methods("POST")
Expand Down Expand Up @@ -1725,43 +1727,47 @@ func addChannelMember(c *Context, w http.ResponseWriter, r *http.Request) {
}

props := model.StringInterfaceFromJSON(r.Body)
userId, ok := props["user_id"].(string)
if !ok || !model.IsValidId(userId) {
c.SetInvalidParam("user_id")
return
}

auditRec := c.MakeAuditRecord("addChannelMember", audit.Fail)
defer c.LogAuditRec(auditRec)
audit.AddEventParameter(auditRec, "user_id", userId)
audit.AddEventParameter(auditRec, "channel_id", c.Params.ChannelId)

member := &model.ChannelMember{
ChannelId: c.Params.ChannelId,
UserId: userId,
var userIds []string
interfaceIds, ok := props["user_ids"].([]interface{})
if ok {
if len(interfaceIds) > maxListSize {
c.SetInvalidParam("user_ids")
return
}
for _, userId := range interfaceIds {
userIds = append(userIds, userId.(string))
}
} else {
userId, ok2 := props["user_id"].(string)
if !ok2 || !model.IsValidId(userId) {
c.SetInvalidParam("user_id or user_ids")
return
}
userIds = append(userIds, userId)
}

postRootId, ok := props["post_root_id"].(string)
if ok && postRootId != "" && !model.IsValidId(postRootId) {
c.SetInvalidParam("post_root_id")
return
}

audit.AddEventParameter(auditRec, "post_root_id", postRootId)
if ok && postRootId != "" {
if !model.IsValidId(postRootId) {
c.SetInvalidParam("post_root_id")
return
}

if ok && len(postRootId) == 26 {
rootPost, err := c.App.GetSinglePost(c.AppContext, postRootId, false)
if err != nil {
c.Err = err
return
}
if rootPost.ChannelId != member.ChannelId {
if rootPost.ChannelId != c.Params.ChannelId {
c.SetInvalidParam("post_root_id")
return
}
} else if !ok {
postRootId = ""
}

channel, err := c.App.GetChannel(c.AppContext, member.ChannelId)
channel, err := c.App.GetChannel(c.AppContext, c.Params.ChannelId)
if err != nil {
c.Err = err
return
Expand All @@ -1772,54 +1778,28 @@ func addChannelMember(c *Context, w http.ResponseWriter, r *http.Request) {
return
}

isNewMembership := false
if _, err = c.App.GetChannelMember(c.AppContext, member.ChannelId, member.UserId); err != nil {
if err.Id == app.MissingChannelMemberError {
isNewMembership = true
} else {
c.Err = err
return
}
}

isSelfAdd := member.UserId == c.AppContext.Session().UserId

canAddSelf := false
canAddOthers := false
if channel.Type == model.ChannelTypeOpen {
if isSelfAdd && isNewMembership {
if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), channel.TeamId, model.PermissionJoinPublicChannels) {
c.SetPermissionError(model.PermissionJoinPublicChannels)
return
}
} else if isSelfAdd && !isNewMembership {
// nothing to do, since already in the channel
} else if !isSelfAdd {
if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), channel.Id, model.PermissionManagePublicChannelMembers) {
c.SetPermissionError(model.PermissionManagePublicChannelMembers)
return
}
if c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), channel.TeamId, model.PermissionJoinPublicChannels) {
canAddSelf = true
}
if c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), channel.Id, model.PermissionManagePublicChannelMembers) {
canAddOthers = true
}
}

if channel.Type == model.ChannelTypePrivate {
if isSelfAdd && isNewMembership {
if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), channel.Id, model.PermissionManagePrivateChannelMembers) {
c.SetPermissionError(model.PermissionManagePrivateChannelMembers)
return
}
} else if isSelfAdd && !isNewMembership {
// nothing to do, since already in the channel
} else if !isSelfAdd {
if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), channel.Id, model.PermissionManagePrivateChannelMembers) {
c.SetPermissionError(model.PermissionManagePrivateChannelMembers)
return
}
if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), channel.Id, model.PermissionManagePrivateChannelMembers) {
c.SetPermissionError(model.PermissionManagePrivateChannelMembers)
return
}
}

if channel.IsGroupConstrained() {
nonMembers, err := c.App.FilterNonGroupChannelMembers([]string{member.UserId}, channel)
nonMembers, err := c.App.FilterNonGroupChannelMembers(userIds, channel)
if err != nil {
if v, ok := err.(*model.AppError); ok {
if v, ok2 := err.(*model.AppError); ok2 {
c.Err = v
} else {
c.Err = model.NewAppError("addChannelMember", "api.channel.add_members.error", nil, "", http.StatusBadRequest).Wrap(err)
Expand All @@ -1832,32 +1812,98 @@ func addChannelMember(c *Context, w http.ResponseWriter, r *http.Request) {
}
}

cm, err := c.App.AddChannelMember(c.AppContext, member.UserId, channel, app.ChannelMemberOpts{
UserRequestorID: c.AppContext.Session().UserId,
PostRootID: postRootId,
})
if err != nil {
c.Err = err
return
}
var lastError *model.AppError
var newChannelMembers []model.ChannelMember
for _, userId := range userIds {
if !model.IsValidId(userId) {
c.Logger.Warn("Error adding channel member, invalid UserId", mlog.String("UserId", userId), mlog.String("ChannelId", channel.Id))
c.SetInvalidParam("user_id")
lastError = c.Err
continue
}

if postRootId != "" {
err := c.App.UpdateThreadFollowForUserFromChannelAdd(c.AppContext, cm.UserId, channel.TeamId, postRootId)
auditRec := c.MakeAuditRecord("addChannelMember", audit.Fail)
defer c.LogAuditRec(auditRec)
audit.AddEventParameter(auditRec, "user_id", userId)
audit.AddEventParameter(auditRec, "channel_id", c.Params.ChannelId)
audit.AddEventParameter(auditRec, "post_root_id", postRootId)

member := &model.ChannelMember{
ChannelId: c.Params.ChannelId,
UserId: userId,
}

existingMember, err := c.App.GetChannelMember(c.AppContext, member.ChannelId, member.UserId)
if err != nil {
c.Err = err
return
if err.Id != app.MissingChannelMemberError {
c.Logger.Warn("Error adding channel member, error getting channel member", mlog.String("UserId", userId), mlog.String("ChannelId", channel.Id), mlog.Err(err))
lastError = err
continue
}
} else {
// user is already a member, go to next
c.Logger.Warn("User is already a channel member, skipping", mlog.String("UserId", userId), mlog.String("ChannelId", channel.Id))
newChannelMembers = append(newChannelMembers, *existingMember)
continue
}

if channel.Type == model.ChannelTypeOpen {
isSelfAdd := member.UserId == c.AppContext.Session().UserId
if isSelfAdd && !canAddSelf {
c.Logger.Warn("Error adding channel member, Invalid Permission to add self", mlog.String("UserId", userId), mlog.String("ChannelId", channel.Id))
c.SetPermissionError(model.PermissionJoinPublicChannels)
lastError = c.Err
continue
} else if !isSelfAdd && !canAddOthers {
c.Logger.Warn("Error adding channel member, Invalid Permission to add others", mlog.String("UserId", userId), mlog.String("ChannelId", channel.Id))
c.SetPermissionError(model.PermissionManagePublicChannelMembers)
lastError = c.Err
continue
}
}

cm, err := c.App.AddChannelMember(c.AppContext, member.UserId, channel, app.ChannelMemberOpts{
UserRequestorID: c.AppContext.Session().UserId,
PostRootID: postRootId,
})
if err != nil {
c.Logger.Warn("Error adding channel member", mlog.String("UserId", userId), mlog.String("ChannelId", channel.Id), mlog.Err(err))
lastError = err
continue
}
newChannelMembers = append(newChannelMembers, *cm)

if postRootId != "" {
err := c.App.UpdateThreadFollowForUserFromChannelAdd(c.AppContext, cm.UserId, channel.TeamId, postRootId)
if err != nil {
c.Logger.Warn("Error adding channel member, error updating thread", mlog.String("UserId", userId), mlog.String("ChannelId", channel.Id), mlog.Err(err))
lastError = err
continue
}
}

auditRec.Success()
auditRec.AddEventResultState(cm)
auditRec.AddEventObjectType("channel_member")
auditRec.AddMeta("add_user_id", cm.UserId)
c.LogAudit("name=" + channel.Name + " user_id=" + cm.UserId)
}

auditRec.Success()
auditRec.AddEventResultState(cm)
auditRec.AddEventObjectType("channel_member")
auditRec.AddMeta("add_user_id", cm.UserId)
c.LogAudit("name=" + channel.Name + " user_id=" + cm.UserId)
if lastError != nil && len(newChannelMembers) == 0 {
c.Err = lastError
return
}

w.WriteHeader(http.StatusCreated)
if err := json.NewEncoder(w).Encode(cm); err != nil {
c.Logger.Warn("Error while writing response", mlog.Err(err))
userId, ok := props["user_id"]
if ok && len(newChannelMembers) == 1 && newChannelMembers[0].UserId == userId {
if err := json.NewEncoder(w).Encode(newChannelMembers[0]); err != nil {
c.Logger.Warn("Error while writing response", mlog.Err(err))
}
} else {
if err := json.NewEncoder(w).Encode(newChannelMembers); err != nil {
c.Logger.Warn("Error while writing response", mlog.Err(err))
}
}
}

Expand Down
37 changes: 37 additions & 0 deletions server/channels/api4/channel_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3294,6 +3294,36 @@ func TestAddChannelMember(t *testing.T) {
})
}

func TestAddChannelMembers(t *testing.T) {
th := Setup(t).InitBasic()
defer th.TearDown()
client := th.Client
user := th.BasicUser
user2 := th.BasicUser2
team := th.BasicTeam
publicChannel := th.CreatePublicChannel()
privateChannel := th.CreatePrivateChannel()

user3 := th.CreateUserWithClient(th.SystemAdminClient)
_, _, err := th.SystemAdminClient.AddTeamMember(context.Background(), team.Id, user3.Id)
require.NoError(t, err)

cm, resp, err := client.AddChannelMembers(context.Background(), publicChannel.Id, "", []string{user.Id, user2.Id, user3.Id})
require.NoError(t, err)
CheckCreatedStatus(t, resp)
require.Equal(t, publicChannel.Id, cm[0].ChannelId, "should have returned exact channel")
require.Equal(t, user.Id, cm[0].UserId, "should have returned exact user added to public channel")
require.Equal(t, user2.Id, cm[1].UserId, "should have returned exact user added to public channel")
require.Equal(t, user3.Id, cm[2].UserId, "should have returned exact user added to public channel")

cm, _, err = client.AddChannelMembers(context.Background(), privateChannel.Id, "", []string{user.Id, user2.Id, user3.Id})
require.NoError(t, err)
require.Equal(t, privateChannel.Id, cm[0].ChannelId, "should have returned exact channel")
require.Equal(t, user.Id, cm[0].UserId, "should have returned exact user added to public channel")
require.Equal(t, user2.Id, cm[1].UserId, "should have returned exact user added to public channel")
require.Equal(t, user3.Id, cm[2].UserId, "should have returned exact user added to public channel")
}

func TestAddChannelMemberFromThread(t *testing.T) {
t.Skip("MM-41285")
th := Setup(t).InitBasic()
Expand Down Expand Up @@ -4784,6 +4814,13 @@ func TestGetChannelsMemberCount(t *testing.T) {
_, _, err := client.GetChannelsMemberCount(context.Background(), channelIDs)
require.NoError(t, err)
})

t.Run("Should fail for private channels that the user is not a member of", func(t *testing.T) {
th.LoginBasic2()
channelIDs := []string{channel2.Id}
_, _, err := client.GetChannelsMemberCount(context.Background(), channelIDs)
require.Error(t, err)
})
}

func TestMoveChannel(t *testing.T) {
Expand Down
17 changes: 17 additions & 0 deletions server/public/model/client4.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3730,6 +3730,23 @@ func (c *Client4) AddChannelMember(ctx context.Context, channelId, userId string
return ch, BuildResponse(r), nil
}

// AddChannelMembers adds users to a channel and return an array of channel members.
func (c *Client4) AddChannelMembers(ctx context.Context, channelId, postRootId string, userIds []string) ([]*ChannelMember, *Response, error) {
requestBody := map[string]any{"user_ids": userIds, "post_root_id": postRootId}
r, err := c.DoAPIPost(ctx, c.channelMembersRoute(channelId)+"", StringInterfaceToJSON(requestBody))
if err != nil {
return nil, BuildResponse(r), err
}
defer closeBody(r)

var ch []*ChannelMember
err = json.NewDecoder(r.Body).Decode(&ch)
if err != nil {
return nil, BuildResponse(r), NewAppError("AddChannelMembers", "api.marshal_error", nil, "", http.StatusInternalServerError).Wrap(err)
}
return ch, BuildResponse(r), nil
}

// AddChannelMemberWithRootId adds user to channel and return a channel member. Post add to channel message has the postRootId.
func (c *Client4) AddChannelMemberWithRootId(ctx context.Context, channelId, userId, postRootId string) (*ChannelMember, *Response, error) {
requestBody := map[string]string{"user_id": userId, "post_root_id": postRootId}
Expand Down
Loading

0 comments on commit 6fd8949

Please sign in to comment.