From 4b9643737027d93498e34398e861492e62a6d2c8 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Thu, 27 Jun 2019 09:37:03 -0400 Subject: [PATCH] MM-16477 Add api to get users modified since a given time (#11406) * MM-16477 Add api to get users modified since a given time * Address feedback --- api4/user.go | 19 ++++++- api4/user_test.go | 81 ++++++++++++++++++++++-------- app/channel.go | 4 +- app/slack.go | 2 +- app/user.go | 17 ++++--- app/user_viewmembers_test.go | 6 ++- model/client4.go | 20 ++++++++ model/user_get.go | 5 ++ store/sqlstore/user_store.go | 20 ++++++-- store/store.go | 13 ++++- store/storetest/mocks/UserStore.go | 10 ++-- store/storetest/user_store.go | 26 ++++++++-- 12 files changed, 176 insertions(+), 47 deletions(-) diff --git a/api4/user.go b/api4/user.go index dd73c8e7ea..633a4d10ad 100644 --- a/api4/user.go +++ b/api4/user.go @@ -15,6 +15,7 @@ import ( "github.com/mattermost/mattermost-server/app" "github.com/mattermost/mattermost-server/mlog" "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/store" "github.com/mattermost/mattermost-server/utils" ) @@ -615,13 +616,29 @@ func getUsersByIds(c *Context, w http.ResponseWriter, r *http.Request) { return } + sinceString := r.URL.Query().Get("since") + + options := &store.UserGetByIdsOpts{ + IsAdmin: c.IsSystemAdmin(), + } + + if len(sinceString) > 0 { + since, parseError := strconv.ParseInt(sinceString, 10, 64) + if parseError != nil { + c.SetInvalidParam("since") + return + } + options.Since = since + } + restrictions, err := c.App.GetViewUsersRestrictions(c.App.Session.UserId) if err != nil { c.Err = err return } + options.ViewRestrictions = restrictions - users, err := c.App.GetUsersByIds(userIds, c.IsSystemAdmin(), restrictions) + users, err := c.App.GetUsersByIds(userIds, options) if err != nil { c.Err = err return diff --git a/api4/user_test.go b/api4/user_test.go index 88529936ac..62a952037a 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -1099,32 +1099,71 @@ func TestGetUsersByIds(t *testing.T) { th := Setup().InitBasic() defer th.TearDown() - users, resp := th.Client.GetUsersByIds([]string{th.BasicUser.Id}) - CheckNoError(t, resp) + t.Run("should return the user", func(t *testing.T) { + users, resp := th.Client.GetUsersByIds([]string{th.BasicUser.Id}) - if users[0].Id != th.BasicUser.Id { - t.Fatal("returned wrong user") - } - CheckUserSanitization(t, users[0]) + CheckNoError(t, resp) - _, resp = th.Client.GetUsersByIds([]string{}) - CheckBadRequestStatus(t, resp) + assert.Equal(t, th.BasicUser.Id, users[0].Id) + CheckUserSanitization(t, users[0]) + }) - users, resp = th.Client.GetUsersByIds([]string{"junk"}) - CheckNoError(t, resp) - if len(users) > 0 { - t.Fatal("no users should be returned") - } + t.Run("should return error when no IDs are specified", func(t *testing.T) { + _, resp := th.Client.GetUsersByIds([]string{}) - users, resp = th.Client.GetUsersByIds([]string{"junk", th.BasicUser.Id}) - CheckNoError(t, resp) - if len(users) != 1 { - t.Fatal("1 user should be returned") - } + CheckBadRequestStatus(t, resp) + }) - th.Client.Logout() - _, resp = th.Client.GetUsersByIds([]string{th.BasicUser.Id}) - CheckUnauthorizedStatus(t, resp) + t.Run("should not return an error for invalid IDs", func(t *testing.T) { + users, resp := th.Client.GetUsersByIds([]string{"junk"}) + + CheckNoError(t, resp) + if len(users) > 0 { + t.Fatal("no users should be returned") + } + }) + + t.Run("should still return users for valid IDs when invalid IDs are specified", func(t *testing.T) { + users, resp := th.Client.GetUsersByIds([]string{"junk", th.BasicUser.Id}) + + CheckNoError(t, resp) + if len(users) != 1 { + t.Fatal("1 user should be returned") + } + }) + + t.Run("should return error when not logged in", func(t *testing.T) { + th.Client.Logout() + + _, resp := th.Client.GetUsersByIds([]string{th.BasicUser.Id}) + CheckUnauthorizedStatus(t, resp) + }) +} + +func TestGetUsersByIdsWithOptions(t *testing.T) { + t.Run("should only return specified users that have been updated since the given time", func(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + // Users before the timestamp shouldn't be returned + user1, err := th.App.CreateUser(&model.User{Email: th.GenerateTestEmail(), Username: model.NewId(), Password: model.NewId()}) + require.Nil(t, err) + + user2, err := th.App.CreateUser(&model.User{Email: th.GenerateTestEmail(), Username: model.NewId(), Password: model.NewId()}) + require.Nil(t, err) + + // Users not in the list of IDs shouldn't be returned + _, err = th.App.CreateUser(&model.User{Email: th.GenerateTestEmail(), Username: model.NewId(), Password: model.NewId()}) + require.Nil(t, err) + + users, resp := th.Client.GetUsersByIdsWithOptions([]string{user1.Id, user2.Id}, &model.UserGetByIdsOptions{ + Since: user2.UpdateAt - 1, + }) + + assert.Nil(t, resp.Error) + assert.Len(t, users, 1) + assert.Equal(t, users[0].Id, user2.Id) + }) } func TestGetUsersByGroupChannelIds(t *testing.T) { diff --git a/app/channel.go b/app/channel.go index 2fa07a88b9..2da1a52569 100644 --- a/app/channel.go +++ b/app/channel.go @@ -434,7 +434,7 @@ func (a *App) createGroupChannel(userIds []string, creatorId string) (*model.Cha return nil, model.NewAppError("CreateGroupChannel", "api.channel.create_group.bad_size.app_error", nil, "", http.StatusBadRequest) } - result := <-a.Srv.Store.User().GetProfileByIds(userIds, true, nil) + result := <-a.Srv.Store.User().GetProfileByIds(userIds, nil, true) if result.Err != nil { return nil, result.Err } @@ -483,7 +483,7 @@ func (a *App) GetGroupChannel(userIds []string) (*model.Channel, *model.AppError return nil, model.NewAppError("GetGroupChannel", "api.channel.create_group.bad_size.app_error", nil, "", http.StatusBadRequest) } - result := <-a.Srv.Store.User().GetProfileByIds(userIds, true, nil) + result := <-a.Srv.Store.User().GetProfileByIds(userIds, nil, true) if result.Err != nil { return nil, result.Err } diff --git a/app/slack.go b/app/slack.go index 45b1391709..e55dd853da 100644 --- a/app/slack.go +++ b/app/slack.go @@ -66,7 +66,7 @@ func replaceUserIds(userStore store.UserStore, text string) string { userIds = append(userIds, match[1]) } - if res := <-userStore.GetProfileByIds(userIds, true, nil); res.Err == nil { + if res := <-userStore.GetProfileByIds(userIds, nil, true); res.Err == nil { for _, user := range res.Data.([]*model.User) { text = strings.Replace(text, "<@"+user.Id+">", "@"+user.Username, -1) } diff --git a/app/user.go b/app/user.go index 9402a93b49..16eed8b2e4 100644 --- a/app/user.go +++ b/app/user.go @@ -617,12 +617,15 @@ func (a *App) GetChannelGroupUsers(channelID string) ([]*model.User, *model.AppE return a.Srv.Store.User().GetChannelGroupUsers(channelID) } -func (a *App) GetUsersByIds(userIds []string, asAdmin bool, viewRestrictions *model.ViewUsersRestrictions) ([]*model.User, *model.AppError) { - result := <-a.Srv.Store.User().GetProfileByIds(userIds, viewRestrictions == nil, viewRestrictions) +func (a *App) GetUsersByIds(userIds []string, options *store.UserGetByIdsOpts) ([]*model.User, *model.AppError) { + allowFromCache := options.ViewRestrictions == nil + + result := <-a.Srv.Store.User().GetProfileByIds(userIds, options, allowFromCache) if result.Err != nil { return nil, result.Err } - return a.sanitizeProfiles(result.Data.([]*model.User), asAdmin), nil + + return a.sanitizeProfiles(result.Data.([]*model.User), options.IsAdmin), nil } func (a *App) GetUsersByGroupChannelIds(channelIds []string, asAdmin bool) (map[string][]*model.User, *model.AppError) { @@ -1713,7 +1716,7 @@ func (a *App) esSearchUsersInTeam(teamId, term string, options *model.UserSearch return nil, err } - result := <-a.Srv.Store.User().GetProfileByIds(usersIds, false, nil) + result := <-a.Srv.Store.User().GetProfileByIds(usersIds, nil, false) if result.Err != nil { return nil, result.Err } @@ -1800,8 +1803,8 @@ func (a *App) esAutocompleteUsersInChannel(teamId, channelId, term string, optio if err != nil { return nil, err } - uchan := a.Srv.Store.User().GetProfileByIds(uchanIds, false, nil) - nuchan := a.Srv.Store.User().GetProfileByIds(nuchanIds, false, nil) + uchan := a.Srv.Store.User().GetProfileByIds(uchanIds, nil, false) + nuchan := a.Srv.Store.User().GetProfileByIds(nuchanIds, nil, false) autocomplete := &model.UserAutocompleteInChannel{} result := <-uchan @@ -1890,7 +1893,7 @@ func (a *App) esAutocompleteUsersInTeam(teamId, term string, options *model.User return nil, err } - result := <-a.Srv.Store.User().GetProfileByIds(usersIds, false, nil) + result := <-a.Srv.Store.User().GetProfileByIds(usersIds, nil, false) if result.Err != nil { return nil, result.Err } diff --git a/app/user_viewmembers_test.go b/app/user_viewmembers_test.go index d2ddfae8f1..7a87b28177 100644 --- a/app/user_viewmembers_test.go +++ b/app/user_viewmembers_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/store" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -828,7 +829,10 @@ func TestResctrictedViewMembers(t *testing.T) { for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { - results, err := th.App.GetUsersByIds(tc.UserIds, false, tc.Restrictions) + results, err := th.App.GetUsersByIds(tc.UserIds, &store.UserGetByIdsOpts{ + IsAdmin: false, + ViewRestrictions: tc.Restrictions, + }) require.Nil(t, err) ids := []string{} for _, result := range results { diff --git a/model/client4.go b/model/client4.go index 3eaaa1e198..2fe67c93c9 100644 --- a/model/client4.go +++ b/model/client4.go @@ -936,6 +936,26 @@ func (c *Client4) GetUsersByIds(userIds []string) ([]*User, *Response) { return UserListFromJson(r.Body), BuildResponse(r) } +// GetUsersByIds returns a list of users based on the provided user ids. +func (c *Client4) GetUsersByIdsWithOptions(userIds []string, options *UserGetByIdsOptions) ([]*User, *Response) { + v := url.Values{} + if options.Since != 0 { + v.Set("since", fmt.Sprintf("%d", options.Since)) + } + + url := c.GetUsersRoute() + "/ids" + if len(v) > 0 { + url += "?" + v.Encode() + } + + r, err := c.DoApiPost(url, ArrayToJson(userIds)) + if err != nil { + return nil, BuildErrorResponse(r, err) + } + defer closeBody(r) + return UserListFromJson(r.Body), BuildResponse(r) +} + // GetUsersByUsernames returns a list of users based on the provided usernames. func (c *Client4) GetUsersByUsernames(usernames []string) ([]*User, *Response) { r, err := c.DoApiPost(c.GetUsersRoute()+"/usernames", ArrayToJson(usernames)) diff --git a/model/user_get.go b/model/user_get.go index ec72eb4972..d4863fe767 100644 --- a/model/user_get.go +++ b/model/user_get.go @@ -29,3 +29,8 @@ type UserGetOptions struct { // Page size PerPage int } + +type UserGetByIdsOptions struct { + // Since filters the users based on their UpdateAt timestamp. + Since int64 +} diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index 7cbdd2fb8f..1c2701d34a 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -10,6 +10,7 @@ import ( "sort" "strings" + "github.com/Masterminds/squirrel" sq "github.com/Masterminds/squirrel" "github.com/mattermost/gorp" @@ -846,8 +847,12 @@ func (us SqlUserStore) GetNewUsersForTeam(teamId string, offset, limit int, view return users, nil } -func (us SqlUserStore) GetProfileByIds(userIds []string, allowFromCache bool, viewRestrictions *model.ViewUsersRestrictions) store.StoreChannel { +func (us SqlUserStore) GetProfileByIds(userIds []string, options *store.UserGetByIdsOpts, allowFromCache bool) store.StoreChannel { return store.Do(func(result *store.StoreResult) { + if options == nil { + options = &store.UserGetByIdsOpts{} + } + users := []*model.User{} remainingUserIds := make([]string, 0) @@ -856,7 +861,10 @@ func (us SqlUserStore) GetProfileByIds(userIds []string, allowFromCache bool, vi if cacheItem, ok := profileByIdsCache.Get(userId); ok { u := &model.User{} *u = *cacheItem.(*model.User) - users = append(users, u) + + if options.Since == 0 || u.UpdateAt > options.Since { + users = append(users, u) + } } else { remainingUserIds = append(remainingUserIds, userId) } @@ -884,7 +892,13 @@ func (us SqlUserStore) GetProfileByIds(userIds []string, allowFromCache bool, vi }). OrderBy("u.Username ASC") - query = applyViewRestrictionsFilter(query, viewRestrictions, true) + if options.Since > 0 { + query = query.Where(squirrel.Gt(map[string]interface{}{ + "u.UpdateAt": options.Since, + })) + } + + query = applyViewRestrictionsFilter(query, options.ViewRestrictions, true) queryString, args, err := query.ToSql() if err != nil { diff --git a/store/store.go b/store/store.go index ba8c6fec4e..cf0a398bd5 100644 --- a/store/store.go +++ b/store/store.go @@ -268,7 +268,7 @@ type UserStore interface { GetProfilesByUsernames(usernames []string, viewRestrictions *model.ViewUsersRestrictions) StoreChannel GetAllProfiles(options *model.UserGetOptions) StoreChannel GetProfiles(options *model.UserGetOptions) StoreChannel - GetProfileByIds(userId []string, allowFromCache bool, viewRestrictions *model.ViewUsersRestrictions) StoreChannel + GetProfileByIds(userIds []string, options *UserGetByIdsOpts, allowFromCache bool) StoreChannel GetProfileByGroupChannelIdsForUser(userId string, channelIds []string) (map[string][]*model.User, *model.AppError) InvalidatProfileCacheForUser(userId string) GetByEmail(email string) (*model.User, *model.AppError) @@ -628,3 +628,14 @@ type ChannelSearchOpts struct { IncludeDeleted bool ExcludeChannelNames []string } + +type UserGetByIdsOpts struct { + // IsAdmin tracks whether or not the request is being made by an administrator. Does nothing when provided by a client. + IsAdmin bool + + // Restrict to search in a list of teams and channels. Does nothing when provided by a client. + ViewRestrictions *model.ViewUsersRestrictions + + // Since filters the users based on their UpdateAt timestamp. + Since int64 +} diff --git a/store/storetest/mocks/UserStore.go b/store/storetest/mocks/UserStore.go index 6abce282b6..cb9320d06b 100644 --- a/store/storetest/mocks/UserStore.go +++ b/store/storetest/mocks/UserStore.go @@ -447,13 +447,13 @@ func (_m *UserStore) GetProfileByGroupChannelIdsForUser(userId string, channelId return r0, r1 } -// GetProfileByIds provides a mock function with given fields: userId, allowFromCache, viewRestrictions -func (_m *UserStore) GetProfileByIds(userId []string, allowFromCache bool, viewRestrictions *model.ViewUsersRestrictions) store.StoreChannel { - ret := _m.Called(userId, allowFromCache, viewRestrictions) +// GetProfileByIds provides a mock function with given fields: userIds, options, allowFromCache +func (_m *UserStore) GetProfileByIds(userIds []string, options *store.UserGetByIdsOpts, allowFromCache bool) store.StoreChannel { + ret := _m.Called(userIds, options, allowFromCache) var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func([]string, bool, *model.ViewUsersRestrictions) store.StoreChannel); ok { - r0 = rf(userId, allowFromCache, viewRestrictions) + if rf, ok := ret.Get(0).(func([]string, *store.UserGetByIdsOpts, bool) store.StoreChannel); ok { + r0 = rf(userIds, options, allowFromCache) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(store.StoreChannel) diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 13bd8bde36..d494c1e8f5 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -1187,35 +1187,51 @@ func testUserStoreGetProfilesByIds(t *testing.T, ss store.Store) { u3.IsBot = true defer func() { require.Nil(t, ss.Bot().PermanentDelete(u3.Id)) }() + u4 := store.Must(ss.User().Save(&model.User{ + Email: MakeEmail(), + Username: "u4" + model.NewId(), + })).(*model.User) + defer func() { require.Nil(t, ss.User().PermanentDelete(u4.Id)) }() + t.Run("get u1 by id, no caching", func(t *testing.T) { - result := <-ss.User().GetProfileByIds([]string{u1.Id}, false, nil) + result := <-ss.User().GetProfileByIds([]string{u1.Id}, nil, false) require.Nil(t, result.Err) assert.Equal(t, []*model.User{sanitized(u1)}, result.Data.([]*model.User)) }) t.Run("get u1 by id, caching", func(t *testing.T) { - result := <-ss.User().GetProfileByIds([]string{u1.Id}, true, nil) + result := <-ss.User().GetProfileByIds([]string{u1.Id}, nil, true) require.Nil(t, result.Err) assert.Equal(t, []*model.User{sanitized(u1)}, result.Data.([]*model.User)) }) t.Run("get u1, u2, u3 by id, no caching", func(t *testing.T) { - result := <-ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, false, nil) + result := <-ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, nil, false) require.Nil(t, result.Err) assert.Equal(t, []*model.User{sanitized(u1), sanitized(u2), sanitized(u3)}, result.Data.([]*model.User)) }) t.Run("get u1, u2, u3 by id, caching", func(t *testing.T) { - result := <-ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, true, nil) + result := <-ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id}, nil, true) require.Nil(t, result.Err) assert.Equal(t, []*model.User{sanitized(u1), sanitized(u2), sanitized(u3)}, result.Data.([]*model.User)) }) t.Run("get unknown id, caching", func(t *testing.T) { - result := <-ss.User().GetProfileByIds([]string{"123"}, true, nil) + result := <-ss.User().GetProfileByIds([]string{"123"}, nil, true) require.Nil(t, result.Err) assert.Equal(t, []*model.User{}, result.Data.([]*model.User)) }) + + t.Run("should only return users with UpdateAt greater than the since time", func(t *testing.T) { + result := <-ss.User().GetProfileByIds([]string{u1.Id, u2.Id, u3.Id, u4.Id}, &store.UserGetByIdsOpts{ + Since: u2.CreateAt, + }, true) + require.Nil(t, result.Err) + + // u3 comes from the cache, and u4 does not + assert.Equal(t, []*model.User{sanitized(u3), sanitized(u4)}, result.Data.([]*model.User)) + }) } func testUserStoreGetProfileByGroupChannelIdsForUser(t *testing.T, ss store.Store) {