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

* check guest access to other members * lint fix (cherry picked from commit 134422d) Co-authored-by: Scott Bishel <[email protected]> (cherry picked from commit c120e12)
mattermost · Sep 27, 2023 · 8209b6b · 8209b6b
1 parent 4c579d3
commit 8209b6b
Show file tree

Hide file tree

Showing 5 changed files with 127 additions and 15 deletions.
diff --git a/server/api/users.go b/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

diff --git a/server/client/client.go b/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)
 }

diff --git a/server/integrationtests/pluginteststore.go b/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 {

diff --git a/server/integrationtests/user_test.go b/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()

diff --git a/server/services/store/mattermostauthlayer/mattermostauthlayer.go b/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)