From 384d0eb245e2aa325fdcfe2d8ee22a40c91af589 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Mon, 6 Jun 2016 13:03:56 -0400 Subject: PLT-2559 Always return successful when trying to join a channel that the user is already a member of (#3265) * Added unit tests for SqlChannelStore.GetMember * Fixed api routes for accessing channels by name when the name includes an underscore * Changed join channel API to always return successful when the user is already a member of the channel --- api/api.go | 4 +-- api/channel.go | 5 ++++ api/channel_test.go | 32 ++++++++++++++++++++-- store/sql_channel_store_test.go | 60 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 97 insertions(+), 4 deletions(-) diff --git a/api/api.go b/api/api.go index 63a460dc5..3404e0c0b 100644 --- a/api/api.go +++ b/api/api.go @@ -25,7 +25,7 @@ type Routes struct { Channels *mux.Router // 'api/v3/teams/{team_id:[A-Za-z0-9]+}/channels' NeedChannel *mux.Router // 'api/v3/teams/{team_id:[A-Za-z0-9]+}/channels/{channel_id:[A-Za-z0-9]+}' - NeedChannelName *mux.Router // 'api/v3/teams/{team_id:[A-Za-z0-9]+}/channels/name/{channel_name:[A-Za-z0-9-]+}' + NeedChannelName *mux.Router // 'api/v3/teams/{team_id:[A-Za-z0-9]+}/channels/name/{channel_name:[A-Za-z0-9_-]+}' Posts *mux.Router // 'api/v3/teams/{team_id:[A-Za-z0-9]+}/channels/{channel_id:[A-Za-z0-9]+}/posts' NeedPost *mux.Router // 'api/v3/teams/{team_id:[A-Za-z0-9]+}/channels/{channel_id:[A-Za-z0-9]+}/posts/{post_id:[A-Za-z0-9]+}' @@ -60,7 +60,7 @@ func InitApi() { BaseRoutes.NeedTeam = BaseRoutes.Teams.PathPrefix("/{team_id:[A-Za-z0-9]+}").Subrouter() BaseRoutes.Channels = BaseRoutes.NeedTeam.PathPrefix("/channels").Subrouter() BaseRoutes.NeedChannel = BaseRoutes.Channels.PathPrefix("/{channel_id:[A-Za-z0-9]+}").Subrouter() - BaseRoutes.NeedChannelName = BaseRoutes.Channels.PathPrefix("/name/{channel_name:[A-Za-z0-9-]+}").Subrouter() + BaseRoutes.NeedChannelName = BaseRoutes.Channels.PathPrefix("/name/{channel_name:[A-Za-z0-9_-]+}").Subrouter() BaseRoutes.Posts = BaseRoutes.NeedChannel.PathPrefix("/posts").Subrouter() BaseRoutes.NeedPost = BaseRoutes.Posts.PathPrefix("/{post_id:[A-Za-z0-9]+}").Subrouter() BaseRoutes.Commands = BaseRoutes.NeedTeam.PathPrefix("/commands").Subrouter() diff --git a/api/channel.go b/api/channel.go index 6d1604900..e0428f311 100644 --- a/api/channel.go +++ b/api/channel.go @@ -476,6 +476,11 @@ func joinChannel(c *Context, channelChannel store.StoreChannel, userChannel stor channel := cresult.Data.(*model.Channel) user := uresult.Data.(*model.User) + if mresult := <-Srv.Store.Channel().GetMember(channel.Id, user.Id); mresult.Err == nil && mresult.Data != nil { + // the user is already in the channel so just return successful + return nil, channel + } + if !c.HasPermissionsToTeam(channel.TeamId, "join") { return c.Err, nil } diff --git a/api/channel_test.go b/api/channel_test.go index 175b0a14a..5c51e4d93 100644 --- a/api/channel_test.go +++ b/api/channel_test.go @@ -462,11 +462,25 @@ func TestJoinChannelById(t *testing.T) { user3 := th.CreateUser(th.BasicClient) LinkUserToTeam(user3, team) - Client.Login(user3.Email, "pwd") + Client.Must(Client.Login(user3.Email, "Password1")) if _, err := Client.JoinChannel(rchannel.Id); err == nil { t.Fatal("shoudn't be able to join direct channel") } + + th.LoginBasic() + + if _, err := Client.JoinChannel(channel1.Id); err != nil { + t.Fatal("should be able to join public channel that we're a member of") + } + + if _, err := Client.JoinChannel(channel3.Id); err != nil { + t.Fatal("should be able to join private channel that we're a member of") + } + + if _, err := Client.JoinChannel(rchannel.Id); err != nil { + t.Fatal("should be able to join direct channel that we're a member of") + } } func TestJoinChannelByName(t *testing.T) { @@ -492,11 +506,25 @@ func TestJoinChannelByName(t *testing.T) { user3 := th.CreateUser(th.BasicClient) LinkUserToTeam(user3, team) - Client.Login(user3.Email, "pwd") + Client.Must(Client.Login(user3.Email, "Password1")) if _, err := Client.JoinChannelByName(rchannel.Name); err == nil { t.Fatal("shoudn't be able to join direct channel") } + + th.LoginBasic() + + if _, err := Client.JoinChannelByName(channel1.Name); err != nil { + t.Fatal("should be able to join public channel that we're a member of") + } + + if _, err := Client.JoinChannelByName(channel3.Name); err != nil { + t.Fatal("should be able to join private channel that we're a member of") + } + + if _, err := Client.JoinChannelByName(rchannel.Name); err != nil { + t.Fatal("should be able to join direct channel that we're a member of") + } } func TestLeaveChannel(t *testing.T) { diff --git a/store/sql_channel_store_test.go b/store/sql_channel_store_test.go index 1b3ea6fe5..2b284d06e 100644 --- a/store/sql_channel_store_test.go +++ b/store/sql_channel_store_test.go @@ -778,6 +778,66 @@ func TestChannelStoreIncrementMentionCount(t *testing.T) { } } +func TestGetMember(t *testing.T) { + Setup() + + userId := model.NewId() + + c1 := &model.Channel{ + TeamId: model.NewId(), + DisplayName: model.NewId(), + Name: model.NewId(), + Type: model.CHANNEL_OPEN, + } + Must(store.Channel().Save(c1)) + + c2 := &model.Channel{ + TeamId: c1.TeamId, + DisplayName: model.NewId(), + Name: model.NewId(), + Type: model.CHANNEL_OPEN, + } + Must(store.Channel().Save(c2)) + + m1 := &model.ChannelMember{ + ChannelId: c1.Id, + UserId: userId, + NotifyProps: model.GetDefaultChannelNotifyProps(), + } + Must(store.Channel().SaveMember(m1)) + + m2 := &model.ChannelMember{ + ChannelId: c2.Id, + UserId: userId, + NotifyProps: model.GetDefaultChannelNotifyProps(), + } + Must(store.Channel().SaveMember(m2)) + + if result := <-store.Channel().GetMember(model.NewId(), userId); result.Err == nil { + t.Fatal("should've failed to get member for non-existant channel") + } + + if result := <-store.Channel().GetMember(c1.Id, model.NewId()); result.Err == nil { + t.Fatal("should've failed to get member for non-existant user") + } + + if result := <-store.Channel().GetMember(c1.Id, userId); result.Err != nil { + t.Fatal("shouldn't have errored when getting member", result.Err) + } else if member := result.Data.(model.ChannelMember); member.ChannelId != c1.Id { + t.Fatal("should've gotten member of channel 1") + } else if member.UserId != userId { + t.Fatal("should've gotten member for user") + } + + if result := <-store.Channel().GetMember(c2.Id, userId); result.Err != nil { + t.Fatal("shouldn't have errored when getting member", result.Err) + } else if member := result.Data.(model.ChannelMember); member.ChannelId != c2.Id { + t.Fatal("should've gotten member of channel 2") + } else if member.UserId != userId { + t.Fatal("should've gotten member for user") + } +} + func TestGetMemberCount(t *testing.T) { Setup() -- cgit v1.2.3-1-g7c22