MM-54366 Check guest access to other members (#4871) (#4884) (#4888)

* check guest access to other members

* lint fix

(cherry picked from commit 134422df4d)

Co-authored-by: Scott Bishel <scott.bishel@mattermost.com>
(cherry picked from commit c120e12455)

server/api/users.go

@@ -93,15 +93,25 @@ func (a *API) handleGetUsersList(w http.ResponseWriter, r *http.Request) {
 	session := ctx.Value(sessionContextKey).(*model.Session)
 	isSystemAdmin := a.permissions.HasPermissionTo(session.UserID, model.PermissionManageSystem)
 
+	sanitizedUsers := make([]*model.User, 0)
 	for _, user := range users {
+		canSeeUser, err2 := a.app.CanSeeUser(session.UserID, user.ID)
+		if err2 != nil {
+			a.errorResponse(w, r, err2)
+			return
+		}
+		if !canSeeUser {
+			continue
+		}
 		if user.ID == session.UserID {
 			user.Sanitize(map[string]bool{})
 		} else {
 			a.app.SanitizeProfile(user, isSystemAdmin)
 		}
+		sanitizedUsers = append(sanitizedUsers, user)
 	}
 
-	usersList, err := json.Marshal(users)
+	usersList, err := json.Marshal(sanitizedUsers)
 	if err != nil {
 		a.errorResponse(w, r, err)
 		return

server/client/client.go

@@ -634,6 +634,26 @@ func (c *Client) GetUser(id string) (*model.User, *Response) {
 	return user, BuildResponse(r)
 }
 
+func (c *Client) GetUserList(ids []string) ([]model.User, *Response) {
+	r, err := c.DoAPIPost("/users", toJSON(ids))
+	if err != nil {
+		return nil, BuildErrorResponse(r, err)
+	}
+	defer closeBody(r)
+
+	requestBody, err := io.ReadAll(r.Body)
+	if err != nil {
+		return nil, BuildErrorResponse(r, err)
+	}
+
+	var users []model.User
+	err = json.Unmarshal(requestBody, &users)
+	if err != nil {
+		return nil, BuildErrorResponse(r, err)
+	}
+	return users, BuildResponse(r)
+}
+
 func (c *Client) GetUserChangePasswordRoute(id string) string {
 	return fmt.Sprintf("/users/%s/changepassword", id)
 }

server/integrationtests/pluginteststore.go

@@ -127,6 +127,17 @@ func (s *PluginTestStore) GetUserByID(userID string) (*model.User, error) {
 	return user, nil
 }
 
+func (s *PluginTestStore) GetUsersList(userIDs []string, showEmail, showName bool) ([]*model.User, error) {
+	var users []*model.User
+	for _, id := range userIDs {
+		user := s.users[id]
+		if user != nil {
+			users = append(users, user)
+		}
+	}
+	return users, nil
+}
+
 func (s *PluginTestStore) GetUserByEmail(email string) (*model.User, error) {
 	for _, user := range s.users {
 		if user.Email == email {

server/integrationtests/user_test.go

@@ -164,6 +164,79 @@ func TestGetUser(t *testing.T) {
 	})
 }
 
+func TestGetUserList(t *testing.T) {
+	th := SetupTestHelperPluginMode(t)
+	defer th.TearDown()
+	clients := setupClients(th)
+	th.Client = clients.TeamMember
+	th.Client2 = clients.Editor
+
+	me, resp := th.Client.GetMe()
+	require.NoError(t, resp.Error)
+	require.NotNil(t, me)
+
+	userID1 := me.ID
+	userID2 := th.GetUser2().ID
+
+	// Admin user should return both
+	returnUsers, resp := clients.Admin.GetUserList([]string{userID1, userID2})
+	require.NoError(t, resp.Error)
+	require.NotNil(t, returnUsers)
+	require.Equal(t, 2, len(returnUsers))
+
+	// Guest user should return none
+	returnUsers2, resp := clients.Guest.GetUserList([]string{userID1, userID2})
+	require.NoError(t, resp.Error)
+	require.NotNil(t, returnUsers2)
+	require.Equal(t, 0, len(returnUsers2))
+
+	newBoard := &model.Board{
+		Title:  "title",
+		Type:   model.BoardTypeOpen,
+		TeamID: testTeamID,
+	}
+	board, err := th.Server.App().CreateBoard(newBoard, userID1, true)
+	require.NoError(t, err)
+
+	// add Guest as board member
+	newGuestMember := &model.BoardMember{
+		UserID:          userGuestID,
+		BoardID:         board.ID,
+		SchemeViewer:    true,
+		SchemeCommenter: true,
+		SchemeEditor:    true,
+		SchemeAdmin:     false,
+	}
+	guestMember, err := th.Server.App().AddMemberToBoard(newGuestMember)
+	require.NoError(t, err)
+	require.NotNil(t, guestMember)
+
+	// Guest user should now return one of members
+	guestUsers, resp := clients.Guest.GetUserList([]string{th.GetUser1().ID, th.GetUser2().ID})
+	require.NoError(t, resp.Error)
+	require.NotNil(t, guestUsers)
+	require.Equal(t, 1, len(guestUsers))
+
+	// add other user as board member
+	newBoardMember := &model.BoardMember{
+		UserID:          userID2,
+		BoardID:         board.ID,
+		SchemeViewer:    true,
+		SchemeCommenter: true,
+		SchemeEditor:    true,
+		SchemeAdmin:     false,
+	}
+	newMember, err := th.Server.App().AddMemberToBoard(newBoardMember)
+	require.NoError(t, err)
+	require.NotNil(t, newMember)
+
+	// Guest user should now return both
+	guestUsers, resp = clients.Guest.GetUserList([]string{th.GetUser1().ID, th.GetUser2().ID})
+	require.NoError(t, resp.Error)
+	require.NotNil(t, guestUsers)
+	require.Equal(t, 2, len(guestUsers))
+}
+
 func TestUserChangePassword(t *testing.T) {
 	th := SetupTestHelper(t).Start()
 	defer th.TearDown()

server/services/store/mattermostauthlayer/mattermostauthlayer.go

@@ -1297,17 +1297,16 @@ func (s *MattermostAuthLayer) CanSeeUser(seerID string, seenID string) (bool, er
 
 	query := s.getQueryBuilder().
 		Select("1").
-		From(s.tablePrefix + "board_members AS BM1").
-		Join(s.tablePrefix + "board_members AS BM2 ON BM1.BoardID=BM2.BoardID").
-		LeftJoin("Bots b ON ( b.UserId = u.id )").
+		From(s.tablePrefix + "board_members AS bm1").
+		Join(s.tablePrefix + "board_members AS bm2 ON bm1.board_id=bm2.board_id").
 		Where(sq.Or{
 			sq.And{
-				sq.Eq{"BM1.UserID": seerID},
-				sq.Eq{"BM2.UserID": seenID},
+				sq.Eq{"bm1.user_id": seerID},
+				sq.Eq{"bm2.user_id": seenID},
 			},
 			sq.And{
-				sq.Eq{"BM1.UserID": seenID},
-				sq.Eq{"BM2.UserID": seerID},
+				sq.Eq{"bm1.user_id": seenID},
+				sq.Eq{"bm2.user_id": seerID},
 			},
 		}).Limit(1)
 
@@ -1323,17 +1322,16 @@ func (s *MattermostAuthLayer) CanSeeUser(seerID string, seenID string) (bool, er
 
 	query = s.getQueryBuilder().
 		Select("1").
-		From("ChannelMembers AS CM1").
-		Join("ChannelMembers AS CM2 ON CM1.BoardID=CM2.BoardID").
-		LeftJoin("Bots b ON ( b.UserId = u.id )").
+		From("channelmembers AS cm1").
+		Join("channelmembers AS cm2 ON cm1.channelid=cm2.channelid").
 		Where(sq.Or{
 			sq.And{
-				sq.Eq{"CM1.UserID": seerID},
-				sq.Eq{"CM2.UserID": seenID},
+				sq.Eq{"cm1.userid": seerID},
+				sq.Eq{"cm2.userid": seenID},
 			},
 			sq.And{
-				sq.Eq{"CM1.UserID": seenID},
-				sq.Eq{"CM2.UserID": seerID},
+				sq.Eq{"cm1.userid": seenID},
+				sq.Eq{"cm2.userid": seerID},
 			},
 		}).Limit(1)