From d45a54a8e9e7bc6c5ec0cf2bde61c267c6fc1b63 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Tue, 24 Sep 2024 13:01:19 +0530 Subject: [PATCH] MM-60480: Do not invalidate channel member cache on login (#28143) On user login, we were aggressively clearing the channel member cache when it wasn't necessary. There is no channel membership info that is changing on a user login. Additionally, we fix some more issues with pointer passing to the cache. It's a known problem that our value passing style isn't consistent when we fetch items from cache. Sometimes we pass pointer-to-pointer, sometimes it's just a pointer. This is also the reason for https://github.com/mattermost/mattermost/pull/27830. We fix 2 such cases where we passed pointer-to-pointer but didn't handle the special case inside the cache. This time, we actually fix it in the app layer instead of the cache layer and also add a test for good measure. https://mattermost.atlassian.net/browse/MM-60480 ```release-note NONE ``` Co-authored-by: Mattermost Build --- server/channels/app/authentication.go | 8 ++------ server/channels/app/platform/session.go | 6 ++++-- .../store/localcachelayer/channel_layer.go | 2 +- .../localcachelayer/channel_layer_test.go | 20 +++++++++++++++++++ 4 files changed, 27 insertions(+), 9 deletions(-) diff --git a/server/channels/app/authentication.go b/server/channels/app/authentication.go index d113e25fde..0a53ad41ef 100644 --- a/server/channels/app/authentication.go +++ b/server/channels/app/authentication.go @@ -64,13 +64,13 @@ func (a *App) CheckPasswordAndAllCriteria(rctx request.CTX, user *model.User, pa return err } + defer a.Srv().Store().User().InvalidateProfileCacheForUser(user.Id) + if err := users.CheckUserPassword(user, password); err != nil { if passErr := a.Srv().Store().User().UpdateFailedPasswordAttempts(user.Id, user.FailedAttempts+1); passErr != nil { return model.NewAppError("CheckPasswordAndAllCriteria", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(passErr) } - a.InvalidateCacheForUser(user.Id) - var invErr *users.ErrInvalidPassword switch { case errors.As(err, &invErr): @@ -89,8 +89,6 @@ func (a *App) CheckPasswordAndAllCriteria(rctx request.CTX, user *model.User, pa } } - a.InvalidateCacheForUser(user.Id) - return err } @@ -98,8 +96,6 @@ func (a *App) CheckPasswordAndAllCriteria(rctx request.CTX, user *model.User, pa return model.NewAppError("CheckPasswordAndAllCriteria", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(passErr) } - a.InvalidateCacheForUser(user.Id) - if err := a.CheckUserPostflightAuthenticationCriteria(rctx, user); err != nil { return err } diff --git a/server/channels/app/platform/session.go b/server/channels/app/platform/session.go index 889d26c9f4..0b0bf495e8 100644 --- a/server/channels/app/platform/session.go +++ b/server/channels/app/platform/session.go @@ -65,7 +65,9 @@ func (ps *PlatformService) ClearUserSessionCacheLocal(userID string) { toPass := make([]any, 0, len(keys)) for i := 0; i < len(keys); i++ { - var session *model.Session + // This always needs to be a pointer to a value. + // Otherwise the msp unmarshaler will fail to work. + var session model.Session toPass = append(toPass, &session) } @@ -77,7 +79,7 @@ func (ps *PlatformService) ClearUserSessionCacheLocal(userID string) { } continue } - gotSession := *(toPass[i].(**model.Session)) + gotSession := toPass[i].(*model.Session) if gotSession == nil { ps.logger.Warn("Found nil session in ClearUserSessionCacheLocal. This is not expected") continue diff --git a/server/channels/store/localcachelayer/channel_layer.go b/server/channels/store/localcachelayer/channel_layer.go index 8fb2106caf..4e3bd1f173 100644 --- a/server/channels/store/localcachelayer/channel_layer.go +++ b/server/channels/store/localcachelayer/channel_layer.go @@ -291,7 +291,7 @@ func (s LocalCacheChannelStore) GetAllChannelMembersForUser(ctx request.CTX, use cache_key += "_deleted" } if allowFromCache { - ids := make(map[string]string) + var ids model.StringMap if err := s.rootStore.doStandardReadCache(s.rootStore.channelMembersForUserCache, cache_key, &ids); err == nil { return ids, nil } diff --git a/server/channels/store/localcachelayer/channel_layer_test.go b/server/channels/store/localcachelayer/channel_layer_test.go index 1a8afe34ea..3ad69b693b 100644 --- a/server/channels/store/localcachelayer/channel_layer_test.go +++ b/server/channels/store/localcachelayer/channel_layer_test.go @@ -10,9 +10,12 @@ import ( "github.com/stretchr/testify/require" "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/plugin/plugintest/mock" "github.com/mattermost/mattermost/server/public/shared/mlog" + "github.com/mattermost/mattermost/server/public/shared/request" "github.com/mattermost/mattermost/server/v8/channels/store/storetest" "github.com/mattermost/mattermost/server/v8/channels/store/storetest/mocks" + cmocks "github.com/mattermost/mattermost/server/v8/platform/services/cache/mocks" ) func TestChannelStore(t *testing.T) { @@ -417,3 +420,20 @@ func TestChannelStoreGetByNamesCache(t *testing.T) { mockStore.Channel().(*mocks.ChannelStore).AssertNumberOfCalls(t, "GetByNames", 2) }) } + +func TestChannelStoreGetAllChannelMembersForUser(t *testing.T) { + logger := mlog.CreateConsoleTestLogger(t) + + mockStore := getMockStore(t) + mockCacheProvider := getMockCacheProvider() + cachedStore, err := NewLocalCacheLayer(mockStore, nil, nil, mockCacheProvider, logger) + require.NoError(t, err) + + cmock := cmocks.NewCache(t) + cmock.On("Get", "u1", mock.AnythingOfType("*model.StringMap")).Return(nil) + + cachedStore.channel.rootStore.channelMembersForUserCache = cmock + + _, err = cachedStore.Channel().GetAllChannelMembersForUser(request.TestContext(t), "u1", true, false) + require.NoError(t, err) +}