From 4b01f1cd122ed155ff8b05aa848f4daf2aa60c34 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Thu, 24 Feb 2022 11:09:01 +0000 Subject: [PATCH 1/3] State resolution v2 micro-optimisations (#2226) * Don't populate duplicates into auth events * Only append the single event * Potentially reduce number of iterations in `isInAllAuthLists --- roomserver/state/state.go | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/roomserver/state/state.go b/roomserver/state/state.go index e5f69521e..187b996cd 100644 --- a/roomserver/state/state.go +++ b/roomserver/state/state.go @@ -814,6 +814,7 @@ func (v *StateResolution) resolveConflictsV2( // events may be duplicated across these sets but that's OK. authSets := make(map[string][]*gomatrixserverlib.Event, len(conflicted)) authEvents := make([]*gomatrixserverlib.Event, 0, estimate*3) + gotAuthEvents := make(map[string]struct{}, estimate*3) authDifference := make([]*gomatrixserverlib.Event, 0, estimate) // For each conflicted event, let's try and get the needed auth events. @@ -850,9 +851,22 @@ func (v *StateResolution) resolveConflictsV2( if err != nil { return nil, err } - authEvents = append(authEvents, authSets[key]...) + + // Only add auth events into the authEvents slice once, otherwise the + // check for the auth difference can become expensive and produce + // duplicate entries, which just waste memory and CPU time. + for _, event := range authSets[key] { + if _, ok := gotAuthEvents[event.EventID()]; !ok { + authEvents = append(authEvents, event) + gotAuthEvents[event.EventID()] = struct{}{} + } + } } + // Kill the reference to this so that the GC may pick it up, since we no + // longer need this after this point. + gotAuthEvents = nil // nolint:ineffassign + // This function helps us to work out whether an event exists in one of the // auth sets. isInAuthList := func(k string, event *gomatrixserverlib.Event) bool { @@ -866,11 +880,12 @@ func (v *StateResolution) resolveConflictsV2( // This function works out if an event exists in all of the auth sets. isInAllAuthLists := func(event *gomatrixserverlib.Event) bool { - found := true for k := range authSets { - found = found && isInAuthList(k, event) + if !isInAuthList(k, event) { + return false + } } - return found + return true } // Look through all of the auth events that we've been given and work out if From 4c07374c42e5d671cfb137634475f43f84f9db0e Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Thu, 24 Feb 2022 17:05:49 +0000 Subject: [PATCH 2/3] Reduce allocations significantly in state res v2, which should help to keep memory down when joining rooms too (update to matrix-org/gomatrixserverlib@f6ab9c5) --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index b104b4192..a416ec98f 100644 --- a/go.mod +++ b/go.mod @@ -39,7 +39,7 @@ require ( github.com/matrix-org/go-http-js-libp2p v0.0.0-20200518170932-783164aeeda4 github.com/matrix-org/go-sqlite3-js v0.0.0-20210709140738-b0d1ba599a6d github.com/matrix-org/gomatrix v0.0.0-20210324163249-be2af5ef2e16 - github.com/matrix-org/gomatrixserverlib v0.0.0-20220214133635-20632dd262ed + github.com/matrix-org/gomatrixserverlib v0.0.0-20220224170509-f6ab9c54d052 github.com/matrix-org/pinecone v0.0.0-20220223104432-0f0afd1a46aa github.com/matrix-org/util v0.0.0-20200807132607-55161520e1d4 github.com/mattn/go-sqlite3 v1.14.10 diff --git a/go.sum b/go.sum index 23ffe8b85..ecb0496bc 100644 --- a/go.sum +++ b/go.sum @@ -983,8 +983,8 @@ github.com/matrix-org/go-sqlite3-js v0.0.0-20210709140738-b0d1ba599a6d/go.mod h1 github.com/matrix-org/gomatrix v0.0.0-20190528120928-7df988a63f26/go.mod h1:3fxX6gUjWyI/2Bt7J1OLhpCzOfO/bB3AiX0cJtEKud0= github.com/matrix-org/gomatrix v0.0.0-20210324163249-be2af5ef2e16 h1:ZtO5uywdd5dLDCud4r0r55eP4j9FuUNpl60Gmntcop4= github.com/matrix-org/gomatrix v0.0.0-20210324163249-be2af5ef2e16/go.mod h1:/gBX06Kw0exX1HrwmoBibFA98yBk/jxKpGVeyQbff+s= -github.com/matrix-org/gomatrixserverlib v0.0.0-20220214133635-20632dd262ed h1:R8EiLWArq7KT96DrUq1xq9scPh8vLwKKeCTnORPyjhU= -github.com/matrix-org/gomatrixserverlib v0.0.0-20220214133635-20632dd262ed/go.mod h1:qFvhfbQ5orQxlH9vCiFnP4dW27xxnWHdNUBKyj/fbiY= +github.com/matrix-org/gomatrixserverlib v0.0.0-20220224170509-f6ab9c54d052 h1:+4Q/JQ3fGgA7sIHaLMlqREX8yEpsI+HlVoW9WId7SNc= +github.com/matrix-org/gomatrixserverlib v0.0.0-20220224170509-f6ab9c54d052/go.mod h1:+WF5InseAMgi1fTnU46JH39IDpEvLep0fDzx9LDf2Bo= github.com/matrix-org/pinecone v0.0.0-20220223104432-0f0afd1a46aa h1:rMYFNVto66gp+eWS8XAUzgp4m0qmUBid6l1HX3mHstk= github.com/matrix-org/pinecone v0.0.0-20220223104432-0f0afd1a46aa/go.mod h1:r6dsL+ylE0yXe/7zh8y/Bdh6aBYI1r+u4yZni9A4iyk= github.com/matrix-org/util v0.0.0-20190711121626-527ce5ddefc7/go.mod h1:vVQlW/emklohkZnOPwD3LrZUBqdfsbiyO3p1lNV8F6U= From cf27e26712c5aa655377f21a2efe34237c3d681f Mon Sep 17 00:00:00 2001 From: S7evinK <2353100+S7evinK@users.noreply.github.com> Date: Fri, 25 Feb 2022 14:33:02 +0100 Subject: [PATCH 3/3] Remember parameters on registration (#2225) * Remember parameters for sessions Cleanup sessions on successfully registering or after a while * Add flakey test * Update to use time.AfterFunc, add more tests * Try to drain the channel, if possible --- clientapi/routing/auth_fallback.go | 2 +- clientapi/routing/key_crosssigning.go | 2 +- clientapi/routing/password.go | 2 +- clientapi/routing/register.go | 122 ++++++++++++++++++++------ clientapi/routing/register_test.go | 45 +++++++++- sytest-blacklist | 1 + sytest-whitelist | 4 + 7 files changed, 149 insertions(+), 29 deletions(-) diff --git a/clientapi/routing/auth_fallback.go b/clientapi/routing/auth_fallback.go index 839ca9e54..abfe830fb 100644 --- a/clientapi/routing/auth_fallback.go +++ b/clientapi/routing/auth_fallback.go @@ -162,7 +162,7 @@ func AuthFallback( } // Success. Add recaptcha as a completed login flow - AddCompletedSessionStage(sessionID, authtypes.LoginTypeRecaptcha) + sessions.addCompletedSessionStage(sessionID, authtypes.LoginTypeRecaptcha) serveSuccess() return nil diff --git a/clientapi/routing/key_crosssigning.go b/clientapi/routing/key_crosssigning.go index 7ecab9d4e..4426b7fdc 100644 --- a/clientapi/routing/key_crosssigning.go +++ b/clientapi/routing/key_crosssigning.go @@ -70,7 +70,7 @@ func UploadCrossSigningDeviceKeys( if _, authErr := typePassword.Login(req.Context(), &uploadReq.Auth.PasswordRequest); authErr != nil { return *authErr } - AddCompletedSessionStage(sessionID, authtypes.LoginTypePassword) + sessions.addCompletedSessionStage(sessionID, authtypes.LoginTypePassword) uploadReq.UserID = device.UserID keyserverAPI.PerformUploadDeviceKeys(req.Context(), &uploadReq.PerformUploadDeviceKeysRequest, uploadRes) diff --git a/clientapi/routing/password.go b/clientapi/routing/password.go index 499510193..acac60fa5 100644 --- a/clientapi/routing/password.go +++ b/clientapi/routing/password.go @@ -74,7 +74,7 @@ func Password( if _, authErr := typePassword.Login(req.Context(), &r.Auth.PasswordRequest); authErr != nil { return *authErr } - AddCompletedSessionStage(sessionID, authtypes.LoginTypePassword) + sessions.addCompletedSessionStage(sessionID, authtypes.LoginTypePassword) // Check the new password strength. if resErr = validatePassword(r.NewPassword); resErr != nil { diff --git a/clientapi/routing/register.go b/clientapi/routing/register.go index d00d9886e..10cfa4325 100644 --- a/clientapi/routing/register.go +++ b/clientapi/routing/register.go @@ -72,14 +72,19 @@ func init() { // sessionsDict keeps track of completed auth stages for each session. // It shouldn't be passed by value because it contains a mutex. type sessionsDict struct { - sync.Mutex + sync.RWMutex sessions map[string][]authtypes.LoginType + params map[string]registerRequest + timer map[string]*time.Timer } -// GetCompletedStages returns the completed stages for a session. -func (d *sessionsDict) GetCompletedStages(sessionID string) []authtypes.LoginType { - d.Lock() - defer d.Unlock() +// defaultTimeout is the timeout used to clean up sessions +const defaultTimeOut = time.Minute * 5 + +// getCompletedStages returns the completed stages for a session. +func (d *sessionsDict) getCompletedStages(sessionID string) []authtypes.LoginType { + d.RLock() + defer d.RUnlock() if completedStages, ok := d.sessions[sessionID]; ok { return completedStages @@ -88,28 +93,79 @@ func (d *sessionsDict) GetCompletedStages(sessionID string) []authtypes.LoginTyp return make([]authtypes.LoginType, 0) } -func newSessionsDict() *sessionsDict { - return &sessionsDict{ - sessions: make(map[string][]authtypes.LoginType), +// addParams adds a registerRequest to a sessionID and starts a timer to delete that registerRequest +func (d *sessionsDict) addParams(sessionID string, r registerRequest) { + d.startTimer(defaultTimeOut, sessionID) + d.Lock() + defer d.Unlock() + d.params[sessionID] = r +} + +func (d *sessionsDict) getParams(sessionID string) (registerRequest, bool) { + d.RLock() + defer d.RUnlock() + r, ok := d.params[sessionID] + return r, ok +} + +// deleteSession cleans up a given session, either because the registration completed +// successfully, or because a given timeout (default: 5min) was reached. +func (d *sessionsDict) deleteSession(sessionID string) { + d.Lock() + defer d.Unlock() + delete(d.params, sessionID) + delete(d.sessions, sessionID) + // stop the timer, e.g. because the registration was completed + if t, ok := d.timer[sessionID]; ok { + if !t.Stop() { + select { + case <-t.C: + default: + } + } + delete(d.timer, sessionID) } } -// AddCompletedSessionStage records that a session has completed an auth stage. -func AddCompletedSessionStage(sessionID string, stage authtypes.LoginType) { - sessions.Lock() - defer sessions.Unlock() +func newSessionsDict() *sessionsDict { + return &sessionsDict{ + sessions: make(map[string][]authtypes.LoginType), + params: make(map[string]registerRequest), + timer: make(map[string]*time.Timer), + } +} - for _, completedStage := range sessions.sessions[sessionID] { +func (d *sessionsDict) startTimer(duration time.Duration, sessionID string) { + d.Lock() + defer d.Unlock() + t, ok := d.timer[sessionID] + if ok { + if !t.Stop() { + <-t.C + } + t.Reset(duration) + return + } + d.timer[sessionID] = time.AfterFunc(duration, func() { + d.deleteSession(sessionID) + }) +} + +// addCompletedSessionStage records that a session has completed an auth stage +// also starts a timer to delete the session once done. +func (d *sessionsDict) addCompletedSessionStage(sessionID string, stage authtypes.LoginType) { + d.startTimer(defaultTimeOut, sessionID) + d.Lock() + defer d.Unlock() + for _, completedStage := range d.sessions[sessionID] { if completedStage == stage { return } } - sessions.sessions[sessionID] = append(sessions.sessions[sessionID], stage) + d.sessions[sessionID] = append(sessions.sessions[sessionID], stage) } var ( - // TODO: Remove old sessions. Need to do so on a session-specific timeout. - // sessions stores the completed flow stages for all sessions. Referenced using their sessionID. sessions = newSessionsDict() validUsernameRegex = regexp.MustCompile(`^[0-9a-z_\-=./]+$`) ) @@ -167,7 +223,7 @@ func newUserInteractiveResponse( params map[string]interface{}, ) userInteractiveResponse { return userInteractiveResponse{ - fs, sessions.GetCompletedStages(sessionID), params, sessionID, + fs, sessions.getCompletedStages(sessionID), params, sessionID, } } @@ -645,12 +701,12 @@ func handleRegistrationFlow( } // Add Recaptcha to the list of completed registration stages - AddCompletedSessionStage(sessionID, authtypes.LoginTypeRecaptcha) + sessions.addCompletedSessionStage(sessionID, authtypes.LoginTypeRecaptcha) case authtypes.LoginTypeDummy: // there is nothing to do // Add Dummy to the list of completed registration stages - AddCompletedSessionStage(sessionID, authtypes.LoginTypeDummy) + sessions.addCompletedSessionStage(sessionID, authtypes.LoginTypeDummy) case "": // An empty auth type means that we want to fetch the available @@ -666,7 +722,7 @@ func handleRegistrationFlow( // Check if the user's registration flow has been completed successfully // A response with current registration flow and remaining available methods // will be returned if a flow has not been successfully completed yet - return checkAndCompleteFlow(sessions.GetCompletedStages(sessionID), + return checkAndCompleteFlow(sessions.getCompletedStages(sessionID), req, r, sessionID, cfg, userAPI) } @@ -708,7 +764,7 @@ func handleApplicationServiceRegistration( // Don't need to worry about appending to registration stages as // application service registration is entirely separate. return completeRegistration( - req.Context(), userAPI, r.Username, "", appserviceID, req.RemoteAddr, req.UserAgent(), + req.Context(), userAPI, r.Username, "", appserviceID, req.RemoteAddr, req.UserAgent(), r.Auth.Session, r.InhibitLogin, r.InitialDisplayName, r.DeviceID, userapi.AccountTypeAppService, ) } @@ -727,11 +783,11 @@ func checkAndCompleteFlow( if checkFlowCompleted(flow, cfg.Derived.Registration.Flows) { // This flow was completed, registration can continue return completeRegistration( - req.Context(), userAPI, r.Username, r.Password, "", req.RemoteAddr, req.UserAgent(), + req.Context(), userAPI, r.Username, r.Password, "", req.RemoteAddr, req.UserAgent(), sessionID, r.InhibitLogin, r.InitialDisplayName, r.DeviceID, userapi.AccountTypeUser, ) } - + sessions.addParams(sessionID, r) // There are still more stages to complete. // Return the flows and those that have been completed. return util.JSONResponse{ @@ -750,11 +806,25 @@ func checkAndCompleteFlow( func completeRegistration( ctx context.Context, userAPI userapi.UserInternalAPI, - username, password, appserviceID, ipAddr, userAgent string, + username, password, appserviceID, ipAddr, userAgent, sessionID string, inhibitLogin eventutil.WeakBoolean, displayName, deviceID *string, accType userapi.AccountType, ) util.JSONResponse { + var registrationOK bool + defer func() { + if registrationOK { + sessions.deleteSession(sessionID) + } + }() + + if data, ok := sessions.getParams(sessionID); ok { + username = data.Username + password = data.Password + deviceID = data.DeviceID + displayName = data.InitialDisplayName + inhibitLogin = data.InhibitLogin + } if username == "" { return util.JSONResponse{ Code: http.StatusBadRequest, @@ -795,6 +865,7 @@ func completeRegistration( // Check whether inhibit_login option is set. If so, don't create an access // token or a device for this user if inhibitLogin { + registrationOK = true return util.JSONResponse{ Code: http.StatusOK, JSON: registerResponse{ @@ -828,6 +899,7 @@ func completeRegistration( } } + registrationOK = true return util.JSONResponse{ Code: http.StatusOK, JSON: registerResponse{ @@ -976,5 +1048,5 @@ func handleSharedSecretRegistration(userAPI userapi.UserInternalAPI, sr *SharedS if ssrr.Admin { accType = userapi.AccountTypeAdmin } - return completeRegistration(req.Context(), userAPI, ssrr.User, ssrr.Password, "", req.RemoteAddr, req.UserAgent(), false, &ssrr.User, &deviceID, accType) + return completeRegistration(req.Context(), userAPI, ssrr.User, ssrr.Password, "", req.RemoteAddr, req.UserAgent(), "", false, &ssrr.User, &deviceID, accType) } diff --git a/clientapi/routing/register_test.go b/clientapi/routing/register_test.go index 1f615dc26..c6b7e61cf 100644 --- a/clientapi/routing/register_test.go +++ b/clientapi/routing/register_test.go @@ -17,6 +17,7 @@ package routing import ( "regexp" "testing" + "time" "github.com/matrix-org/dendrite/clientapi/auth/authtypes" "github.com/matrix-org/dendrite/setup/config" @@ -140,7 +141,7 @@ func TestFlowCheckingExtraneousIncorrectInput(t *testing.T) { func TestEmptyCompletedFlows(t *testing.T) { fakeEmptySessions := newSessionsDict() fakeSessionID := "aRandomSessionIDWhichDoesNotExist" - ret := fakeEmptySessions.GetCompletedStages(fakeSessionID) + ret := fakeEmptySessions.getCompletedStages(fakeSessionID) // check for [] if ret == nil || len(ret) != 0 { @@ -208,3 +209,45 @@ func TestValidationOfApplicationServices(t *testing.T) { t.Errorf("user_id should not have been valid: @_something_else:localhost") } } + +func TestSessionCleanUp(t *testing.T) { + s := newSessionsDict() + + t.Run("session is cleaned up after a while", func(t *testing.T) { + t.Parallel() + dummySession := "helloWorld" + // manually added, as s.addParams() would start the timer with the default timeout + s.params[dummySession] = registerRequest{Username: "Testing"} + s.startTimer(time.Millisecond, dummySession) + time.Sleep(time.Millisecond * 2) + if data, ok := s.getParams(dummySession); ok { + t.Errorf("expected session to be deleted: %+v", data) + } + }) + + t.Run("session is deleted, once the registration completed", func(t *testing.T) { + t.Parallel() + dummySession := "helloWorld2" + s.startTimer(time.Minute, dummySession) + s.deleteSession(dummySession) + if data, ok := s.getParams(dummySession); ok { + t.Errorf("expected session to be deleted: %+v", data) + } + }) + + t.Run("session timer is restarted after second call", func(t *testing.T) { + t.Parallel() + dummySession := "helloWorld3" + // the following will start a timer with the default timeout of 5min + s.addParams(dummySession, registerRequest{Username: "Testing"}) + s.addCompletedSessionStage(dummySession, authtypes.LoginTypeRecaptcha) + s.addCompletedSessionStage(dummySession, authtypes.LoginTypeDummy) + s.getCompletedStages(dummySession) + // reset the timer with a lower timeout + s.startTimer(time.Millisecond, dummySession) + time.Sleep(time.Millisecond * 2) + if data, ok := s.getParams(dummySession); ok { + t.Errorf("expected session to be deleted: %+v", data) + } + }) +} \ No newline at end of file diff --git a/sytest-blacklist b/sytest-blacklist index 16abce8da..e8617dcdf 100644 --- a/sytest-blacklist +++ b/sytest-blacklist @@ -24,6 +24,7 @@ Local device key changes get to remote servers with correct prev_id # Flakey Local device key changes appear in /keys/changes +/context/ with lazy_load_members filter works # we don't support groups Remove group category diff --git a/sytest-whitelist b/sytest-whitelist index d3144572d..12522cfb3 100644 --- a/sytest-whitelist +++ b/sytest-whitelist @@ -601,3 +601,7 @@ Can query remote device keys using POST after notification Device deletion propagates over federation Get left notifs in sync and /keys/changes when other user leaves Remote banned user is kicked and may not rejoin until unbanned +registration remembers parameters +registration accepts non-ascii passwords +registration with inhibit_login inhibits login +