[GH-7494] Added the role to the user search filter (#9976)

* 7494 added the role to the user search filter * 7494 changed the getUser function to accept the options * added the role filter for the getAllProfiles method * 7494 added the Inactive filter for AllProfiles * 7494 refactored the where clause generation * 7494 added the roles and inactive filters for inTeam Query * 7494 fixed the review comments
2025-02-25 18:55:24 -06:00 · 2019-01-11 14:50:32 +01:00
parent 09a519799f
commit bbee234af0
11 changed files with 321 additions and 70 deletions
--- a/store/sqlstore/user_store.go
+++ b/store/sqlstore/user_store.go
@@ -364,11 +364,23 @@ func (s SqlUserStore) GetEtagForAllProfiles() store.StoreChannel {
 	})
 }

-func (us SqlUserStore) GetAllProfiles(offset int, limit int) store.StoreChannel {
+func (us SqlUserStore) GetAllProfiles(options *model.UserGetOptions) store.StoreChannel {
+	isPostgreSQL := us.DriverName() == model.DATABASE_DRIVER_POSTGRES
 	return store.Do(func(result *store.StoreResult) {
 		var users []*model.User
+		offset := options.Page * options.PerPage
+		limit := options.PerPage

-		if _, err := us.GetReplica().Select(&users, "SELECT * FROM Users ORDER BY Username ASC LIMIT :Limit OFFSET :Offset", map[string]interface{}{"Offset": offset, "Limit": limit}); err != nil {
+		searchQuery := `
+			SELECT * FROM Users
+			WHERE_CONDITION
+			ORDER BY Username ASC LIMIT :Limit OFFSET :Offset
+		`
+
+		parameters := map[string]interface{}{"Offset": offset, "Limit": limit}
+		searchQuery = substituteWhereClause(searchQuery, options, parameters, isPostgreSQL)
+
+		if _, err := us.GetReplica().Select(&users, searchQuery, parameters); err != nil {
 			result.Err = model.NewAppError("SqlUserStore.GetAllProfiles", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError)
 		} else {

@@ -381,6 +393,37 @@ func (us SqlUserStore) GetAllProfiles(offset int, limit int) store.StoreChannel
 	})
 }

+func substituteWhereClause(searchQuery string, options *model.UserGetOptions, parameters map[string]interface{}, isPostgreSQL bool) string {
+	whereClause := ""
+	whereClauses := []string{}
+	if options.Role != "" {
+		whereClauses = append(whereClauses, getRoleFilter(isPostgreSQL))
+		parameters["Role"] = fmt.Sprintf("%%%s%%", options.Role)
+	}
+	if options.Inactive {
+		whereClauses = append(whereClauses, " Users.DeleteAt != 0 ")
+	}
+
+	if len(whereClauses) > 0 {
+		whereClause = strings.Join(whereClauses, " AND ")
+		searchQuery = strings.Replace(searchQuery, "WHERE_CONDITION", fmt.Sprintf(" WHERE %s ", whereClause), 1)
+		searchQuery = strings.Replace(searchQuery, "SEARCH_CLAUSE", fmt.Sprintf(" AND %s ", whereClause), 1)
+	} else {
+		searchQuery = strings.Replace(searchQuery, "WHERE_CONDITION", "", 1)
+		searchQuery = strings.Replace(searchQuery, "SEARCH_CLAUSE", "", 1)
+	}
+
+	return searchQuery
+}
+
+func getRoleFilter(isPostgreSQL bool) string {
+	if isPostgreSQL {
+		return fmt.Sprintf("Users.Roles like lower(%s)", ":Role")
+	} else {
+		return fmt.Sprintf("Users.Roles LIKE %s escape '*' ", ":Role")
+	}
+}
+
 func (s SqlUserStore) GetEtagForProfiles(teamId string) store.StoreChannel {
 	return store.Do(func(result *store.StoreResult) {
 		updateAt, err := s.GetReplica().SelectInt("SELECT UpdateAt FROM Users, TeamMembers WHERE TeamMembers.TeamId = :TeamId AND Users.Id = TeamMembers.UserId ORDER BY UpdateAt DESC LIMIT 1", map[string]interface{}{"TeamId": teamId})
@@ -392,11 +435,26 @@ func (s SqlUserStore) GetEtagForProfiles(teamId string) store.StoreChannel {
 	})
 }

-func (us SqlUserStore) GetProfiles(teamId string, offset int, limit int) store.StoreChannel {
+func (us SqlUserStore) GetProfiles(options *model.UserGetOptions) store.StoreChannel {
+	isPostgreSQL := us.DriverName() == model.DATABASE_DRIVER_POSTGRES
+	teamId := options.InTeamId
+	offset := options.Page * options.PerPage
+	limit := options.PerPage
+
+	searchQuery := `
+		SELECT Users.* FROM Users, TeamMembers 
+		WHERE TeamMembers.TeamId = :TeamId AND Users.Id = TeamMembers.UserId AND TeamMembers.DeleteAt = 0
+		SEARCH_CLAUSE
+		ORDER BY Users.Username ASC LIMIT :Limit OFFSET :Offset
+		`
+
+	parameters := map[string]interface{}{"TeamId": teamId, "Offset": offset, "Limit": limit}
+	searchQuery = substituteWhereClause(searchQuery, options, parameters, isPostgreSQL)
+
 	return store.Do(func(result *store.StoreResult) {
 		var users []*model.User

-		if _, err := us.GetReplica().Select(&users, "SELECT Users.* FROM Users, TeamMembers WHERE TeamMembers.TeamId = :TeamId AND Users.Id = TeamMembers.UserId AND TeamMembers.DeleteAt = 0 ORDER BY Users.Username ASC LIMIT :Limit OFFSET :Offset", map[string]interface{}{"TeamId": teamId, "Offset": offset, "Limit": limit}); err != nil {
+		if _, err := us.GetReplica().Select(&users, searchQuery, parameters); err != nil {
 			result.Err = model.NewAppError("SqlUserStore.GetProfiles", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError)
 		} else {

@@ -1154,7 +1212,7 @@ var spaceFulltextSearchChar = []string{
 	"@",
 }

-func generateSearchQuery(searchQuery string, terms []string, fields []string, parameters map[string]interface{}, isPostgreSQL bool) string {
+func generateSearchQuery(searchQuery string, terms []string, fields []string, parameters map[string]interface{}, isPostgreSQL bool, role string) string {
 	searchTerms := []string{}
 	for i, term := range terms {
 		searchFields := []string{}
@@ -1169,6 +1227,11 @@ func generateSearchQuery(searchQuery string, terms []string, fields []string, pa
 		parameters[fmt.Sprintf("Term%d", i)] = fmt.Sprintf("%s%%", strings.TrimLeft(term, "@"))
 	}

+	if role != "" {
+		searchTerms = append(searchTerms, getRoleFilter(isPostgreSQL))
+		parameters["Role"] = fmt.Sprintf("%%%s%%", role)
+	}
+
 	searchClause := strings.Join(searchTerms, " AND ")
 	return strings.Replace(searchQuery, "SEARCH_CLAUSE", fmt.Sprintf(" AND %s ", searchClause), 1)
 }
@@ -1201,6 +1264,11 @@ func (us SqlUserStore) performSearch(searchQuery string, term string, options *m
 		}
 	}

+	role := ""
+	if options.Role != "" {
+		role = options.Role
+	}
+
 	if ok := options.AllowInactive; ok {
 		searchQuery = strings.Replace(searchQuery, "INACTIVE_CLAUSE", "", 1)
 	} else {
@@ -1211,7 +1279,7 @@ func (us SqlUserStore) performSearch(searchQuery string, term string, options *m
 		searchQuery = strings.Replace(searchQuery, "SEARCH_CLAUSE", "", 1)
 	} else {
 		isPostgreSQL := us.DriverName() == model.DATABASE_DRIVER_POSTGRES
-		searchQuery = generateSearchQuery(searchQuery, strings.Fields(term), searchType, parameters, isPostgreSQL)
+		searchQuery = generateSearchQuery(searchQuery, strings.Fields(term), searchType, parameters, isPostgreSQL, role)
 	}

 	var users []*model.User
--- a/store/store.go
+++ b/store/store.go
@@ -249,8 +249,8 @@ type UserStore interface {
 	GetProfilesNotInChannel(teamId string, channelId string, offset int, limit int) StoreChannel
 	GetProfilesWithoutTeam(offset int, limit int) StoreChannel
 	GetProfilesByUsernames(usernames []string, teamId string) StoreChannel
-	GetAllProfiles(offset int, limit int) StoreChannel
-	GetProfiles(teamId string, offset int, limit int) StoreChannel
+	GetAllProfiles(options *model.UserGetOptions) StoreChannel
+	GetProfiles(options *model.UserGetOptions) StoreChannel
 	GetProfileByIds(userId []string, allowFromCache bool) StoreChannel
 	InvalidatProfileCacheForUser(userId string)
 	GetByEmail(email string) StoreChannel
--- a/store/storetest/mocks/UserStore.go
+++ b/store/storetest/mocks/UserStore.go
@@ -146,13 +146,13 @@ func (_m *UserStore) GetAllAfter(limit int, afterId string) store.StoreChannel {
 	return r0
 }

-// GetAllProfiles provides a mock function with given fields: offset, limit
-func (_m *UserStore) GetAllProfiles(offset int, limit int) store.StoreChannel {
-	ret := _m.Called(offset, limit)
+// GetAllProfiles provides a mock function with given fields: options
+func (_m *UserStore) GetAllProfiles(options *model.UserGetOptions) store.StoreChannel {
+	ret := _m.Called(options)

 	var r0 store.StoreChannel
-	if rf, ok := ret.Get(0).(func(int, int) store.StoreChannel); ok {
-		r0 = rf(offset, limit)
+	if rf, ok := ret.Get(0).(func(*model.UserGetOptions) store.StoreChannel); ok {
+		r0 = rf(options)
 	} else {
 		if ret.Get(0) != nil {
 			r0 = ret.Get(0).(store.StoreChannel)
@@ -354,13 +354,13 @@ func (_m *UserStore) GetProfileByIds(userId []string, allowFromCache bool) store
 	return r0
 }

-// GetProfiles provides a mock function with given fields: teamId, offset, limit
-func (_m *UserStore) GetProfiles(teamId string, offset int, limit int) store.StoreChannel {
-	ret := _m.Called(teamId, offset, limit)
+// GetProfiles provides a mock function with given fields: options
+func (_m *UserStore) GetProfiles(options *model.UserGetOptions) store.StoreChannel {
+	ret := _m.Called(options)

 	var r0 store.StoreChannel
-	if rf, ok := ret.Get(0).(func(string, int, int) store.StoreChannel); ok {
-		r0 = rf(teamId, offset, limit)
+	if rf, ok := ret.Get(0).(func(*model.UserGetOptions) store.StoreChannel); ok {
+		r0 = rf(options)
 	} else {
 		if ret.Get(0) != nil {
 			r0 = ret.Get(0).(store.StoreChannel)
--- a/store/storetest/user_store.go
+++ b/store/storetest/user_store.go
@@ -307,7 +307,9 @@ func testUserStoreGetAllProfiles(t *testing.T, ss store.Store) {
 	store.Must(ss.User().Save(u2))
 	defer func() { store.Must(ss.User().PermanentDelete(u2.Id)) }()

-	if r1 := <-ss.User().GetAllProfiles(0, 100); r1.Err != nil {
+	options := &model.UserGetOptions{Page: 0, PerPage: 100}
+
+	if r1 := <-ss.User().GetAllProfiles(options); r1.Err != nil {
 		t.Fatal(r1.Err)
 	} else {
 		users := r1.Data.([]*model.User)
@@ -316,7 +318,8 @@ func testUserStoreGetAllProfiles(t *testing.T, ss store.Store) {
 		}
 	}

-	if r2 := <-ss.User().GetAllProfiles(0, 1); r2.Err != nil {
+	options = &model.UserGetOptions{Page: 0, PerPage: 1}
+	if r2 := <-ss.User().GetAllProfiles(options); r2.Err != nil {
 		t.Fatal(r2.Err)
 	} else {
 		users := r2.Data.([]*model.User)
@@ -343,6 +346,7 @@ func testUserStoreGetAllProfiles(t *testing.T, ss store.Store) {

 	u3 := &model.User{}
 	u3.Email = MakeEmail()
+	u3.Roles = "system_user some-other-role"
 	store.Must(ss.User().Save(u3))
 	defer func() { store.Must(ss.User().PermanentDelete(u3.Id)) }()

@@ -353,6 +357,64 @@ func testUserStoreGetAllProfiles(t *testing.T, ss store.Store) {
 			t.Fatal("etags should not match")
 		}
 	}
+
+	u4 := &model.User{}
+	u4.Email = MakeEmail()
+	u4.Roles = "system_admin some-other-role"
+	store.Must(ss.User().Save(u4))
+	defer func() { store.Must(ss.User().PermanentDelete(u4.Id)) }()
+
+	u5 := &model.User{}
+	u5.Email = MakeEmail()
+	u5.Roles = "system_admin"
+	store.Must(ss.User().Save(u5))
+	defer func() { store.Must(ss.User().PermanentDelete(u5.Id)) }()
+
+	options = &model.UserGetOptions{Page: 0, PerPage: 10, Role: "system_admin"}
+	if r2 := <-ss.User().GetAllProfiles(options); r2.Err != nil {
+		t.Fatal(r2.Err)
+	} else {
+		users := r2.Data.([]*model.User)
+		if len(users) != 2 {
+			t.Fatal("invalid returned users, role filter did not work")
+		}
+		assert.ElementsMatch(t, []string{u4.Id, u5.Id}, []string{users[0].Id, users[1].Id})
+	}
+
+	u6 := &model.User{}
+	u6.Email = MakeEmail()
+	u6.DeleteAt = model.GetMillis()
+	u6.Roles = "system_admin"
+	store.Must(ss.User().Save(u6))
+	defer func() { store.Must(ss.User().PermanentDelete(u6.Id)) }()
+
+	u7 := &model.User{}
+	u7.Email = MakeEmail()
+	u7.DeleteAt = model.GetMillis()
+	store.Must(ss.User().Save(u7))
+	defer func() { store.Must(ss.User().PermanentDelete(u7.Id)) }()
+
+	options = &model.UserGetOptions{Page: 0, PerPage: 10, Role: "system_admin", Inactive: true}
+	if r2 := <-ss.User().GetAllProfiles(options); r2.Err != nil {
+		t.Fatal(r2.Err)
+	} else {
+		users := r2.Data.([]*model.User)
+		if len(users) != 1 {
+			t.Fatal("invalid returned users, Role and Inactive filter did not work")
+		}
+		assert.Equal(t, u6.Id, users[0].Id)
+	}
+
+	options = &model.UserGetOptions{Page: 0, PerPage: 10, Inactive: true}
+	if r2 := <-ss.User().GetAllProfiles(options); r2.Err != nil {
+		t.Fatal(r2.Err)
+	} else {
+		users := r2.Data.([]*model.User)
+		if len(users) != 2 {
+			t.Fatal("invalid returned users, Inactive filter did not work")
+		}
+		assert.ElementsMatch(t, []string{u6.Id, u7.Id}, []string{users[0].Id, users[1].Id})
+	}
 }

 func testUserStoreGetProfiles(t *testing.T, ss store.Store) {
@@ -370,7 +432,8 @@ func testUserStoreGetProfiles(t *testing.T, ss store.Store) {
 	defer func() { store.Must(ss.User().PermanentDelete(u2.Id)) }()
 	store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u2.Id}, -1))

-	if r1 := <-ss.User().GetProfiles(teamId, 0, 100); r1.Err != nil {
+	options := &model.UserGetOptions{InTeamId: teamId, Page: 0, PerPage: 100}
+	if r1 := <-ss.User().GetProfiles(options); r1.Err != nil {
 		t.Fatal(r1.Err)
 	} else {
 		users := r1.Data.([]*model.User)
@@ -390,7 +453,8 @@ func testUserStoreGetProfiles(t *testing.T, ss store.Store) {
 		}
 	}

-	if r2 := <-ss.User().GetProfiles("123", 0, 100); r2.Err != nil {
+	options = &model.UserGetOptions{InTeamId: "123", Page: 0, PerPage: 100}
+	if r2 := <-ss.User().GetProfiles(options); r2.Err != nil {
 		t.Fatal(r2.Err)
 	} else {
 		if len(r2.Data.([]*model.User)) != 0 {
@@ -418,6 +482,53 @@ func testUserStoreGetProfiles(t *testing.T, ss store.Store) {
 			t.Fatal("etags should not match")
 		}
 	}
+
+	u4 := &model.User{}
+	u4.Email = MakeEmail()
+	u4.Roles = "system_admin"
+	store.Must(ss.User().Save(u4))
+	defer func() { store.Must(ss.User().PermanentDelete(u4.Id)) }()
+	store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u4.Id}, -1))
+
+	u5 := &model.User{}
+	u5.Email = MakeEmail()
+	u5.DeleteAt = model.GetMillis()
+	store.Must(ss.User().Save(u5))
+	defer func() { store.Must(ss.User().PermanentDelete(u5.Id)) }()
+	store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u5.Id}, -1))
+
+	options = &model.UserGetOptions{InTeamId: teamId, Page: 0, PerPage: 100}
+	if r1 := <-ss.User().GetProfiles(options); r1.Err != nil {
+		t.Fatal(r1.Err)
+	} else {
+		users := r1.Data.([]*model.User)
+		if len(users) != 5 {
+			t.Fatal("invalid returned users")
+		}
+	}
+
+	options = &model.UserGetOptions{InTeamId: teamId, Role: "system_admin", Inactive: false, Page: 0, PerPage: 100}
+	if r1 := <-ss.User().GetProfiles(options); r1.Err != nil {
+		t.Fatal(r1.Err)
+	} else {
+		users := r1.Data.([]*model.User)
+		if len(users) != 1 {
+			t.Fatal("invalid returned users")
+		}
+		assert.Equal(t, u4.Id, users[0].Id)
+	}
+
+	options = &model.UserGetOptions{InTeamId: teamId, Inactive: true, Page: 0, PerPage: 100}
+	if r1 := <-ss.User().GetProfiles(options); r1.Err != nil {
+		t.Fatal(r1.Err)
+	} else {
+		users := r1.Data.([]*model.User)
+		if len(users) != 1 {
+			t.Fatal("invalid returned users")
+		}
+		assert.Equal(t, u5.Id, users[0].Id)
+	}
+
 }

 func testUserStoreGetProfilesInChannel(t *testing.T, ss store.Store) {
@@ -937,7 +1048,8 @@ func testUserStoreGetProfilesByIds(t *testing.T, ss store.Store) {
 		}
 	}

-	if r2 := <-ss.User().GetProfiles("123", 0, 100); r2.Err != nil {
+	options := &model.UserGetOptions{InTeamId: "123", Page: 0, PerPage: 100}
+	if r2 := <-ss.User().GetProfiles(options); r2.Err != nil {
 		t.Fatal(r2.Err)
 	} else {
 		if len(r2.Data.([]*model.User)) != 0 {
@@ -1410,6 +1522,22 @@ func assertUsers(t *testing.T, expected, actual []*model.User) {
 	}
 }

+func assertUsersMatchInAnyOrder(t *testing.T, expected, actual []*model.User) {
+	expectedUsernames := make([]string, 0, len(expected))
+	for _, user := range expected {
+		expectedUsernames = append(expectedUsernames, user.Username)
+	}
+
+	actualUsernames := make([]string, 0, len(actual))
+	for _, user := range actual {
+		actualUsernames = append(actualUsernames, user.Username)
+	}
+
+	if assert.ElementsMatch(t, expectedUsernames, actualUsernames) {
+		assert.ElementsMatch(t, expected, actual)
+	}
+}
+
 func testUserStoreSearch(t *testing.T, ss store.Store) {
 	u1 := &model.User{
 		Username:  "jimbo1" + model.NewId(),
@@ -1417,6 +1545,7 @@ func testUserStoreSearch(t *testing.T, ss store.Store) {
 		LastName:  "Bill",
 		Nickname:  "Rob",
 		Email:     "harold" + model.NewId() + "@simulator.amazonses.com",
+		Roles:     "system_user system_admin",
 	}
 	store.Must(ss.User().Save(u1))
 	defer func() { store.Must(ss.User().PermanentDelete(u1.Id)) }()
@@ -1424,6 +1553,7 @@ func testUserStoreSearch(t *testing.T, ss store.Store) {
 	u2 := &model.User{
 		Username: "jim-bobby" + model.NewId(),
 		Email:    MakeEmail(),
+		Roles:    "system_user",
 	}
 	store.Must(ss.User().Save(u2))
 	defer func() { store.Must(ss.User().PermanentDelete(u2.Id)) }()
@@ -1432,6 +1562,7 @@ func testUserStoreSearch(t *testing.T, ss store.Store) {
 		Username: "jimbo3" + model.NewId(),
 		Email:    MakeEmail(),
 		DeleteAt: 1,
+		Roles:    "system_admin",
 	}
 	store.Must(ss.User().Save(u3))
 	defer func() { store.Must(ss.User().PermanentDelete(u3.Id)) }()
@@ -1668,13 +1799,46 @@ func testUserStoreSearch(t *testing.T, ss store.Store) {
 			},
 			[]*model.User{u1},
 		},
+		{
+			"search jim-bobby with system_user roles",
+			tid,
+			"jim-bobby",
+			&model.UserSearchOptions{
+				AllowFullNames: true,
+				Limit:          model.USER_SEARCH_DEFAULT_LIMIT,
+				Role:           "system_user",
+			},
+			[]*model.User{u2},
+		},
+		{
+			"search jim with system_admin roles",
+			tid,
+			"jim",
+			&model.UserSearchOptions{
+				AllowFullNames: true,
+				Limit:          model.USER_SEARCH_DEFAULT_LIMIT,
+				Role:           "system_admin",
+			},
+			[]*model.User{u1},
+		},
+		{
+			"search ji with system_user roles",
+			tid,
+			"ji",
+			&model.UserSearchOptions{
+				AllowFullNames: true,
+				Limit:          model.USER_SEARCH_DEFAULT_LIMIT,
+				Role:           "system_user",
+			},
+			[]*model.User{u1, u2},
+		},
 	}

 	for _, testCase := range testCases {
 		t.Run(testCase.Description, func(t *testing.T) {
 			result := <-ss.User().Search(testCase.TeamId, testCase.Term, testCase.Options)
 			require.Nil(t, result.Err)
-			assertUsers(t, testCase.Expected, result.Data.([]*model.User))
+			assertUsersMatchInAnyOrder(t, testCase.Expected, result.Data.([]*model.User))
 		})
 	}