Make LocalCacheUserStore.Get/GetProfileByIds return a copy of the stored data (#13620)

store/localcachelayer/user_layer.go

@@ -105,11 +105,10 @@ func (s LocalCacheUserStore) GetProfileByIds(userIds []string, options *store.Us
 
 	for _, userId := range userIds {
 		if cacheItem := s.rootStore.doStandardReadCache(s.rootStore.userProfileByIdsCache, userId); cacheItem != nil {
-			u := &model.User{}
+			u := cacheItem.(*model.User)
-			*u = *cacheItem.(*model.User)
 
 			if options.Since == 0 || u.UpdateAt > options.Since {
-				users = append(users, u)
+				users = append(users, u.DeepCopy())
 			}
 		} else {
 			remainingUserIds = append(remainingUserIds, userId)
@@ -127,9 +126,8 @@ func (s LocalCacheUserStore) GetProfileByIds(userIds []string, options *store.Us
 			return nil, model.NewAppError("SqlUserStore.GetProfileByIds", "store.sql_user.get_profiles.app_error", nil, err.Error(), http.StatusInternalServerError)
 		}
 
-		users = append(users, remainingUsers...)
-
 		for _, user := range remainingUsers {
+			users = append(users, user.DeepCopy())
 			s.rootStore.doStandardAddToCache(s.rootStore.userProfileByIdsCache, user.Id, user)
 		}
 	}
@@ -147,8 +145,8 @@ func (s LocalCacheUserStore) Get(id string) (*model.User, *model.AppError) {
 		if s.rootStore.metrics != nil {
 			s.rootStore.metrics.AddMemCacheHitCounter("Profile By Id", float64(1))
 		}
-		u := *cacheItem.(*model.User)
+		u := cacheItem.(*model.User)
-		return &u, nil
+		return u.DeepCopy(), nil
 	}
 	if s.rootStore.metrics != nil {
 		s.rootStore.metrics.AddMemCacheMissCounter("Profile By Id", float64(1))
@@ -158,5 +156,5 @@ func (s LocalCacheUserStore) Get(id string) (*model.User, *model.AppError) {
 		return nil, model.NewAppError("SqlUserStore.Get", "store.sql_user.get.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 	s.rootStore.doStandardAddToCache(s.rootStore.userProfileByIdsCache, id, user)
-	return user, nil
+	return user.DeepCopy(), nil
 }

store/localcachelayer/user_layer_test.go

@@ -66,6 +66,50 @@ func TestUserStoreGetProfileByIdsCache(t *testing.T) {
 		_, _ = cachedStore.User().GetProfileByIds(fakeUserIds, &store.UserGetByIdsOpts{}, true)
 		mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "GetProfileByIds", 2)
 	})
+
+	t.Run("should always return a copy of the stored data", func(t *testing.T) {
+		mockStore := getMockStore()
+		mockCacheProvider := getMockCacheProvider()
+		cachedStore := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider)
+
+		storedUsers, err := mockStore.User().GetProfileByIds(fakeUserIds, &store.UserGetByIdsOpts{}, false)
+		require.Nil(t, err)
+
+		originalProps := make([]model.StringMap, len(storedUsers))
+
+		for i := 0; i < len(storedUsers); i++ {
+			originalProps[i] = storedUsers[i].NotifyProps
+			storedUsers[i].NotifyProps = map[string]string{}
+			storedUsers[i].NotifyProps["key"] = "somevalue"
+		}
+
+		cachedUsers, err := cachedStore.User().GetProfileByIds(fakeUserIds, &store.UserGetByIdsOpts{}, true)
+		require.Nil(t, err)
+
+		for i := 0; i < len(storedUsers); i++ {
+			assert.Equal(t, storedUsers[i], cachedUsers[i])
+			if storedUsers[i] == cachedUsers[i] {
+				assert.Fail(t, "should be different pointers")
+			}
+			cachedUsers[i].NotifyProps["key"] = "othervalue"
+			assert.NotEqual(t, storedUsers[i], cachedUsers[i])
+		}
+
+		cachedUsers, err = cachedStore.User().GetProfileByIds(fakeUserIds, &store.UserGetByIdsOpts{}, true)
+		require.Nil(t, err)
+		for i := 0; i < len(storedUsers); i++ {
+			assert.Equal(t, storedUsers[i], cachedUsers[i])
+			if storedUsers[i] == cachedUsers[i] {
+				assert.Fail(t, "should be different pointers")
+			}
+			cachedUsers[i].NotifyProps["key"] = "othervalue"
+			assert.NotEqual(t, storedUsers[i], cachedUsers[i])
+		}
+
+		for i := 0; i < len(storedUsers); i++ {
+			storedUsers[i].NotifyProps = originalProps[i]
+		}
+	})
 }
 
 func TestUserStoreProfilesInChannelCache(t *testing.T) {
@@ -167,4 +211,37 @@ func TestUserStoreGetCache(t *testing.T) {
 		_, _ = cachedStore.User().Get(fakeUserId)
 		mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "Get", 2)
 	})
+
+	t.Run("should always return a copy of the stored data", func(t *testing.T) {
+		mockStore := getMockStore()
+		mockCacheProvider := getMockCacheProvider()
+		cachedStore := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider)
+
+		storedUser, err := mockStore.User().Get(fakeUserId)
+		require.Nil(t, err)
+		originalProps := storedUser.NotifyProps
+
+		storedUser.NotifyProps = map[string]string{}
+		storedUser.NotifyProps["key"] = "somevalue"
+
+		cachedUser, err := cachedStore.User().Get(fakeUserId)
+		require.Nil(t, err)
+		assert.Equal(t, storedUser, cachedUser)
+		if storedUser == cachedUser {
+			assert.Fail(t, "should be different pointers")
+		}
+		cachedUser.NotifyProps["key"] = "othervalue"
+		assert.NotEqual(t, storedUser, cachedUser)
+
+		cachedUser, err = cachedStore.User().Get(fakeUserId)
+		require.Nil(t, err)
+		assert.Equal(t, storedUser, cachedUser)
+		if storedUser == cachedUser {
+			assert.Fail(t, "should be different pointers")
+		}
+		cachedUser.NotifyProps["key"] = "othervalue"
+		assert.NotEqual(t, storedUser, cachedUser)
+
+		storedUser.NotifyProps = originalProps
+	})
 }