From 903085feb84cda8e308085040799a13a51f2c97f Mon Sep 17 00:00:00 2001 From: Micah Thompson Date: Wed, 10 Jul 2019 14:55:17 -0400 Subject: [PATCH] Migrate "User.UpdateMfaActive" to Sync by default (#11566) --- api4/user_test.go | 16 ++++++++-------- services/mfa/mfa.go | 9 ++++----- store/sqlstore/user_store.go | 16 +++++++--------- store/store.go | 2 +- store/storetest/mocks/UserStore.go | 8 ++++---- store/storetest/user_store.go | 6 +++--- 6 files changed, 27 insertions(+), 30 deletions(-) diff --git a/api4/user_test.go b/api4/user_test.go index b5821c63d9..07fff6a122 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -2114,8 +2114,8 @@ func TestUserLoginMFAFlow(t *testing.T) { assert.Nil(t, err) // Fake user has MFA enabled - if result := <-th.Server.Store.User().UpdateMfaActive(th.BasicUser.Id, true); result.Err != nil { - t.Fatal(result.Err) + if err = th.Server.Store.User().UpdateMfaActive(th.BasicUser.Id, true); err != nil { + t.Fatal(err) } if err = th.Server.Store.User().UpdateMfaSecret(th.BasicUser.Id, secret.Secret); err != nil { @@ -2146,8 +2146,8 @@ func TestUserLoginMFAFlow(t *testing.T) { assert.Nil(t, err) // Fake user has MFA enabled - if result := <-th.Server.Store.User().UpdateMfaActive(th.BasicUser.Id, true); result.Err != nil { - t.Fatal(result.Err) + if err = th.Server.Store.User().UpdateMfaActive(th.BasicUser.Id, true); err != nil { + t.Fatal(err) } if err = th.Server.Store.User().UpdateMfaSecret(th.BasicUser.Id, secret.Secret); err != nil { @@ -4352,8 +4352,8 @@ func TestLoginLockout(t *testing.T) { CheckErrorMessage(t, resp, "api.user.check_user_login_attempts.too_many.app_error") // Fake user has MFA enabled - if result := <-th.Server.Store.User().UpdateMfaActive(th.BasicUser2.Id, true); result.Err != nil { - t.Fatal(result.Err) + if err := th.Server.Store.User().UpdateMfaActive(th.BasicUser2.Id, true); err != nil { + t.Fatal(err) } _, resp = th.Client.LoginWithMFA(th.BasicUser2.Email, th.BasicUser2.Password, "000000") CheckErrorMessage(t, resp, "api.user.check_user_mfa.bad_code.app_error") @@ -4367,8 +4367,8 @@ func TestLoginLockout(t *testing.T) { CheckErrorMessage(t, resp, "api.user.check_user_login_attempts.too_many.app_error") // Fake user has MFA disabled - if result := <-th.Server.Store.User().UpdateMfaActive(th.BasicUser2.Id, false); result.Err != nil { - t.Fatal(result.Err) + if err := th.Server.Store.User().UpdateMfaActive(th.BasicUser2.Id, false); err != nil { + t.Fatal(err) } //Check if lock is active diff --git a/services/mfa/mfa.go b/services/mfa/mfa.go index 6b88250226..91c3c942a6 100644 --- a/services/mfa/mfa.go +++ b/services/mfa/mfa.go @@ -99,8 +99,8 @@ func (m *Mfa) Activate(user *model.User, token string) *model.AppError { return model.NewAppError("Activate", "mfa.activate.bad_token.app_error", nil, "", http.StatusUnauthorized) } - if result := <-m.Store.User().UpdateMfaActive(user.Id, true); result.Err != nil { - return model.NewAppError("Activate", "mfa.activate.save_active.app_error", nil, result.Err.Error(), http.StatusInternalServerError) + if err = m.Store.User().UpdateMfaActive(user.Id, true); err != nil { + return model.NewAppError("Activate", "mfa.activate.save_active.app_error", nil, err.Error(), http.StatusInternalServerError) } return nil @@ -111,15 +111,14 @@ func (m *Mfa) Deactivate(userId string) *model.AppError { return err } - achan := m.Store.User().UpdateMfaActive(userId, false) schan := make(chan *model.AppError, 1) go func() { schan <- m.Store.User().UpdateMfaSecret(userId, "") close(schan) }() - if result := <-achan; result.Err != nil { - return model.NewAppError("Deactivate", "mfa.deactivate.save_active.app_error", nil, result.Err.Error(), http.StatusInternalServerError) + if err := m.Store.User().UpdateMfaActive(userId, false); err != nil { + return model.NewAppError("Deactivate", "mfa.deactivate.save_active.app_error", nil, err.Error(), http.StatusInternalServerError) } if err := <-schan; err != nil { diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index 512cedaaa8..ed86bd5dbc 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -303,16 +303,14 @@ func (us SqlUserStore) UpdateMfaSecret(userId, secret string) *model.AppError { return nil } -func (us SqlUserStore) UpdateMfaActive(userId string, active bool) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - updateAt := model.GetMillis() +func (us SqlUserStore) UpdateMfaActive(userId string, active bool) *model.AppError { + updateAt := model.GetMillis() - if _, err := us.GetMaster().Exec("UPDATE Users SET MfaActive = :Active, UpdateAt = :UpdateAt WHERE Id = :UserId", map[string]interface{}{"Active": active, "UpdateAt": updateAt, "UserId": userId}); err != nil { - result.Err = model.NewAppError("SqlUserStore.UpdateMfaActive", "store.sql_user.update_mfa_active.app_error", nil, "id="+userId+", "+err.Error(), http.StatusInternalServerError) - } else { - result.Data = userId - } - }) + if _, err := us.GetMaster().Exec("UPDATE Users SET MfaActive = :Active, UpdateAt = :UpdateAt WHERE Id = :UserId", map[string]interface{}{"Active": active, "UpdateAt": updateAt, "UserId": userId}); err != nil { + return model.NewAppError("SqlUserStore.UpdateMfaActive", "store.sql_user.update_mfa_active.app_error", nil, "id="+userId+", "+err.Error(), http.StatusInternalServerError) + } + + return nil } func (us SqlUserStore) Get(id string) (*model.User, *model.AppError) { diff --git a/store/store.go b/store/store.go index d28548bd25..6c72ea2fa8 100644 --- a/store/store.go +++ b/store/store.go @@ -257,7 +257,7 @@ type UserStore interface { UpdateUpdateAt(userId string) (int64, *model.AppError) UpdateAuthData(userId string, service string, authData *string, email string, resetMfa bool) (string, *model.AppError) UpdateMfaSecret(userId, secret string) *model.AppError - UpdateMfaActive(userId string, active bool) StoreChannel + UpdateMfaActive(userId string, active bool) *model.AppError Get(id string) (*model.User, *model.AppError) GetAll() ([]*model.User, *model.AppError) ClearCaches() diff --git a/store/storetest/mocks/UserStore.go b/store/storetest/mocks/UserStore.go index 8ac1329dd3..a69d614528 100644 --- a/store/storetest/mocks/UserStore.go +++ b/store/storetest/mocks/UserStore.go @@ -1127,15 +1127,15 @@ func (_m *UserStore) UpdateLastPictureUpdate(userId string) *model.AppError { } // UpdateMfaActive provides a mock function with given fields: userId, active -func (_m *UserStore) UpdateMfaActive(userId string, active bool) store.StoreChannel { +func (_m *UserStore) UpdateMfaActive(userId string, active bool) *model.AppError { ret := _m.Called(userId, active) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string, bool) store.StoreChannel); ok { + var r0 *model.AppError + if rf, ok := ret.Get(0).(func(string, bool) *model.AppError); ok { r0 = rf(userId, active) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.AppError) } } diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index e31e1e144e..4a5bf41a02 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -1987,16 +1987,16 @@ func testUserStoreUpdateMfaActive(t *testing.T, ss store.Store) { time.Sleep(100 * time.Millisecond) - if err = (<-ss.User().UpdateMfaActive(u1.Id, true)).Err; err != nil { + if err = ss.User().UpdateMfaActive(u1.Id, true); err != nil { t.Fatal(err) } - if err = (<-ss.User().UpdateMfaActive(u1.Id, false)).Err; err != nil { + if err = ss.User().UpdateMfaActive(u1.Id, false); err != nil { t.Fatal(err) } // should pass, no update will occur though - if err = (<-ss.User().UpdateMfaActive("junk", true)).Err; err != nil { + if err = ss.User().UpdateMfaActive("junk", true); err != nil { t.Fatal(err) } }