From 9fa6b093f31d2774b0978f0e4a0a201c23edd794 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Mon, 8 Apr 2019 11:10:16 +0200 Subject: [PATCH] Fixing permissions checks where related to join public channels (#10511) * Fixing permissions checks where related to join public channels * Addressing PR review comments * Fixing bug * Adding new tests * Addressing PR review comments --- api4/channel.go | 53 ++++++++++++++++++++++-------- api4/channel_test.go | 76 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 14 deletions(-) diff --git a/api4/channel.go b/api4/channel.go index b645a7de9c..c0712f5b55 100644 --- a/api4/channel.go +++ b/api4/channel.go @@ -8,6 +8,7 @@ import ( "github.com/mattermost/mattermost-server/mlog" "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/store" ) func (api *API) InitChannel() { @@ -411,7 +412,7 @@ func getChannel(c *Context, w http.ResponseWriter, r *http.Request) { } if channel.Type == model.CHANNEL_OPEN { - if !c.App.SessionHasPermissionToTeam(c.App.Session, channel.TeamId, model.PERMISSION_READ_PUBLIC_CHANNEL) { + if !c.App.SessionHasPermissionToTeam(c.App.Session, channel.TeamId, model.PERMISSION_READ_PUBLIC_CHANNEL) && !c.App.SessionHasPermissionToChannel(c.App.Session, c.Params.ChannelId, model.PERMISSION_READ_CHANNEL) { c.SetPermissionError(model.PERMISSION_READ_PUBLIC_CHANNEL) return } @@ -800,7 +801,7 @@ func getChannelByName(c *Context, w http.ResponseWriter, r *http.Request) { } if channel.Type == model.CHANNEL_OPEN { - if !c.App.SessionHasPermissionToTeam(c.App.Session, channel.TeamId, model.PERMISSION_READ_PUBLIC_CHANNEL) { + if !c.App.SessionHasPermissionToTeam(c.App.Session, channel.TeamId, model.PERMISSION_READ_PUBLIC_CHANNEL) && !c.App.SessionHasPermissionToChannel(c.App.Session, channel.Id, model.PERMISSION_READ_CHANNEL) { c.SetPermissionError(model.PERMISSION_READ_PUBLIC_CHANNEL) return } @@ -1124,14 +1125,32 @@ func addChannelMember(c *Context, w http.ResponseWriter, r *http.Request) { return } - // Check join permission if adding yourself, otherwise check manage permission + if channel.Type == model.CHANNEL_DIRECT || channel.Type == model.CHANNEL_GROUP { + c.Err = model.NewAppError("addUserToChannel", "api.channel.add_user_to_channel.type.app_error", nil, "", http.StatusBadRequest) + return + } + + isNewMembership := false + if _, err = c.App.GetChannelMember(member.ChannelId, member.UserId); err != nil { + if err.Id == store.MISSING_CHANNEL_MEMBER_ERROR { + isNewMembership = true + } else { + c.Err = err + return + } + } + + isSelfAdd := member.UserId == c.App.Session.UserId + if channel.Type == model.CHANNEL_OPEN { - if member.UserId == c.App.Session.UserId { - if !c.App.SessionHasPermissionToChannel(c.App.Session, channel.Id, model.PERMISSION_JOIN_PUBLIC_CHANNELS) { + if isSelfAdd && isNewMembership { + if !c.App.SessionHasPermissionToTeam(c.App.Session, channel.TeamId, model.PERMISSION_JOIN_PUBLIC_CHANNELS) { c.SetPermissionError(model.PERMISSION_JOIN_PUBLIC_CHANNELS) return } - } else { + } else if isSelfAdd && !isNewMembership { + // nothing to do, since already in the channel + } else if !isSelfAdd { if !c.App.SessionHasPermissionToChannel(c.App.Session, channel.Id, model.PERMISSION_MANAGE_PUBLIC_CHANNEL_MEMBERS) { c.SetPermissionError(model.PERMISSION_MANAGE_PUBLIC_CHANNEL_MEMBERS) return @@ -1139,14 +1158,20 @@ func addChannelMember(c *Context, w http.ResponseWriter, r *http.Request) { } } - if channel.Type == model.CHANNEL_PRIVATE && !c.App.SessionHasPermissionToChannel(c.App.Session, channel.Id, model.PERMISSION_MANAGE_PRIVATE_CHANNEL_MEMBERS) { - c.SetPermissionError(model.PERMISSION_MANAGE_PRIVATE_CHANNEL_MEMBERS) - return - } - - if channel.Type == model.CHANNEL_DIRECT || channel.Type == model.CHANNEL_GROUP { - c.Err = model.NewAppError("addUserToChannel", "api.channel.add_user_to_channel.type.app_error", nil, "", http.StatusBadRequest) - return + if channel.Type == model.CHANNEL_PRIVATE { + if isSelfAdd && isNewMembership { + if !c.App.SessionHasPermissionToChannel(c.App.Session, channel.Id, model.PERMISSION_MANAGE_PRIVATE_CHANNEL_MEMBERS) { + c.SetPermissionError(model.PERMISSION_MANAGE_PRIVATE_CHANNEL_MEMBERS) + return + } + } else if isSelfAdd && !isNewMembership { + // nothing to do, since already in the channel + } else if !isSelfAdd { + if !c.App.SessionHasPermissionToChannel(c.App.Session, channel.Id, model.PERMISSION_MANAGE_PRIVATE_CHANNEL_MEMBERS) { + c.SetPermissionError(model.PERMISSION_MANAGE_PRIVATE_CHANNEL_MEMBERS) + return + } + } } cm, err := c.App.AddChannelMember(member.UserId, channel, c.App.Session.UserId, postRootId, c.App.Session.Id) diff --git a/api4/channel_test.go b/api4/channel_test.go index e70bf69f0a..aaca1212d2 100644 --- a/api4/channel_test.go +++ b/api4/channel_test.go @@ -2023,6 +2023,82 @@ func TestAddChannelMember(t *testing.T) { Client.Logout() } +func TestAddChannelMemberAddMyself(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + Client := th.Client + user := th.CreateUser() + th.LinkUserToTeam(user, th.BasicTeam) + notMemberPublicChannel1 := th.CreatePublicChannel() + notMemberPublicChannel2 := th.CreatePublicChannel() + notMemberPrivateChannel := th.CreatePrivateChannel() + + memberPublicChannel := th.CreatePublicChannel() + memberPrivateChannel := th.CreatePrivateChannel() + th.AddUserToChannel(user, memberPublicChannel) + th.AddUserToChannel(user, memberPrivateChannel) + + testCases := []struct { + Name string + Channel *model.Channel + WithJoinPublicPermission bool + ExpectedError string + }{ + { + "Add myself to a public channel with JOIN_PUBLIC_CHANNEL permission", + notMemberPublicChannel1, + true, + "", + }, + { + "Try to add myself to a private channel with the JOIN_PUBLIC_CHANNEL permission", + notMemberPrivateChannel, + true, + "api.context.permissions.app_error", + }, + { + "Try to add myself to a public channel without the JOIN_PUBLIC_CHANNEL permission", + notMemberPublicChannel2, + false, + "api.context.permissions.app_error", + }, + { + "Add myself a public channel where I'm already a member, not having JOIN_PUBLIC_CHANNEL or MANAGE MEMBERS permission", + memberPublicChannel, + false, + "", + }, + { + "Add myself a private channel where I'm already a member, not having JOIN_PUBLIC_CHANNEL or MANAGE MEMBERS permission", + memberPrivateChannel, + false, + "", + }, + } + Client.Login(user.Email, user.Password) + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + + // Check the appropriate permissions are enforced. + defaultRolePermissions := th.SaveDefaultRolePermissions() + defer func() { + th.RestoreDefaultRolePermissions(defaultRolePermissions) + }() + + if !tc.WithJoinPublicPermission { + th.RemovePermissionFromRole(model.PERMISSION_JOIN_PUBLIC_CHANNELS.Id, model.TEAM_USER_ROLE_ID) + } + + _, resp := Client.AddChannelMember(tc.Channel.Id, user.Id) + if tc.ExpectedError == "" { + CheckNoError(t, resp) + } else { + CheckErrorMessage(t, resp, tc.ExpectedError) + } + }) + } +} + func TestRemoveChannelMember(t *testing.T) { th := Setup().InitBasic() user1 := th.BasicUser