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

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()