From 604e2471356941e955758612746d8a4768585064 Mon Sep 17 00:00:00 2001 From: krjn <51929404+krjn@users.noreply.github.com> Date: Fri, 21 Jun 2019 22:20:27 +0000 Subject: [PATCH] [MM-16514] Migrate Token.GetByName to Sync by Default (#11355) * [MM-16514] Migrate Token.GetByToken to Sync by default * test: use testify * fix: shadowing --- api4/team_test.go | 7 +++---- api4/user_test.go | 28 +++++++++------------------- app/oauth.go | 7 +++---- app/team.go | 14 ++++++-------- app/team_test.go | 5 ++--- app/user.go | 21 +++++++++------------ app/user_test.go | 5 ++--- store/sqlstore/tokens_store.go | 20 +++++++++----------- store/store.go | 2 +- store/storetest/mocks/TokenStore.go | 19 ++++++++++++++----- 10 files changed, 58 insertions(+), 70 deletions(-) diff --git a/api4/team_test.go b/api4/team_test.go index 6ae57144e6..8381e83257 100644 --- a/api4/team_test.go +++ b/api4/team_test.go @@ -1436,9 +1436,8 @@ func TestAddTeamMember(t *testing.T) { t.Fatal("team ids should have matched") } - if result := <-th.App.Srv.Store.Token().GetByToken(token.Token); result.Err == nil { - t.Fatal("The token must be deleted after be used") - } + _, err := th.App.Srv.Store.Token().GetByToken(token.Token) + require.NotNil(t, err, "The token must be deleted after be used") tm, resp = Client.AddTeamMemberFromInvite("junk", "") CheckBadRequestStatus(t, resp) @@ -1495,7 +1494,7 @@ func TestAddTeamMember(t *testing.T) { // Set a team to group-constrained team.GroupConstrained = model.NewBool(true) - _, err := th.App.UpdateTeam(team) + _, err = th.App.UpdateTeam(team) require.Nil(t, err) // Attempt to use a token on a group-constrained team diff --git a/api4/user_test.go b/api4/user_test.go index 7318619d1c..d77cd64605 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -108,13 +108,8 @@ func TestCreateUserWithToken(t *testing.T) { t.Fatal("did not clear roles") } CheckUserSanitization(t, ruser) - if result := <-th.App.Srv.Store.Token().GetByToken(token.Token); result.Err == nil { - t.Fatal("The token must be deleted after be used") - } - - if result := <-th.App.Srv.Store.Token().GetByToken(token.Token); result.Err == nil { - t.Fatal("The token must be deleted after be used") - } + _, err := th.App.Srv.Store.Token().GetByToken(token.Token) + require.NotNil(t, err, "The token must be deleted after being used") if teams, err := th.App.GetTeamsForUser(ruser.Id); err != nil || len(teams) == 0 { t.Fatal("The user must have teams") @@ -215,9 +210,8 @@ func TestCreateUserWithToken(t *testing.T) { t.Fatal("did not clear roles") } CheckUserSanitization(t, ruser) - if result := <-th.App.Srv.Store.Token().GetByToken(token.Token); result.Err == nil { - t.Fatal("The token must be deleted after be used") - } + _, err := th.App.Srv.Store.Token().GetByToken(token.Token) + require.NotNil(t, err, "The token must be deleted after be used") }) } @@ -2239,7 +2233,8 @@ func TestResetPassword(t *testing.T) { if !strings.ContainsAny(resultsMailbox[0].To[0], user.Email) { t.Fatal("Wrong To recipient") } else { - if resultsEmail, err := mailservice.GetMessageFromMailbox(user.Email, resultsMailbox[0].ID); err == nil { + var resultsEmail mailservice.JSONMessageInbucket + if resultsEmail, err = mailservice.GetMessageFromMailbox(user.Email, resultsMailbox[0].ID); err == nil { loc := strings.Index(resultsEmail.Body.Text, "token=") if loc == -1 { t.Log(resultsEmail.Body.Text) @@ -2250,13 +2245,9 @@ func TestResetPassword(t *testing.T) { } } } - var recoveryToken *model.Token - if result := <-th.App.Srv.Store.Token().GetByToken(recoveryTokenString); result.Err != nil { - t.Log(recoveryTokenString) - t.Fatal(result.Err) - } else { - recoveryToken = result.Data.(*model.Token) - } + recoveryToken, err := th.App.Srv.Store.Token().GetByToken(recoveryTokenString) + require.Nil(t, err, "Recovery token not found (%s)", recoveryTokenString) + _, resp = th.Client.ResetPassword(recoveryToken.Token, "") CheckBadRequestStatus(t, resp) _, resp = th.Client.ResetPassword(recoveryToken.Token, "newp") @@ -4180,7 +4171,6 @@ func TestLoginErrorMessage(t *testing.T) { _, resp := th.Client.Logout() CheckNoError(t, resp) - // Email and Username enabled th.App.UpdateConfig(func(cfg *model.Config) { *cfg.EmailSettings.EnableSignInWithEmail = true diff --git a/app/oauth.go b/app/oauth.go index 364b4b0b3b..fc2c2561ea 100644 --- a/app/oauth.go +++ b/app/oauth.go @@ -601,11 +601,10 @@ func (a *App) CreateOAuthStateToken(extra string) (*model.Token, *model.AppError } func (a *App) GetOAuthStateToken(token string) (*model.Token, *model.AppError) { - result := <-a.Srv.Store.Token().GetByToken(token) - if result.Err != nil { - return nil, model.NewAppError("GetOAuthStateToken", "api.oauth.invalid_state_token.app_error", nil, result.Err.Error(), http.StatusBadRequest) + mToken, err := a.Srv.Store.Token().GetByToken(token) + if err != nil { + return nil, model.NewAppError("GetOAuthStateToken", "api.oauth.invalid_state_token.app_error", nil, err.Error(), http.StatusBadRequest) } - mToken := result.Data.(*model.Token) if mToken.Type != model.TOKEN_TYPE_OAUTH { return nil, model.NewAppError("GetOAuthStateToken", "api.oauth.invalid_state_token.app_error", nil, "", http.StatusBadRequest) diff --git a/app/team.go b/app/team.go index b8297d319c..13a573ac96 100644 --- a/app/team.go +++ b/app/team.go @@ -398,12 +398,11 @@ func (a *App) AddUserToTeamByTeamId(teamId string, user *model.User) *model.AppE } func (a *App) AddUserToTeamByToken(userId string, tokenId string) (*model.Team, *model.AppError) { - result := <-a.Srv.Store.Token().GetByToken(tokenId) - if result.Err != nil { - return nil, model.NewAppError("AddUserToTeamByToken", "api.user.create_user.signup_link_invalid.app_error", nil, result.Err.Error(), http.StatusBadRequest) + token, err := a.Srv.Store.Token().GetByToken(tokenId) + if err != nil { + return nil, model.NewAppError("AddUserToTeamByToken", "api.user.create_user.signup_link_invalid.app_error", nil, err.Error(), http.StatusBadRequest) } - token := result.Data.(*model.Token) if token.Type != TOKEN_TYPE_TEAM_INVITATION { return nil, model.NewAppError("AddUserToTeamByToken", "api.user.create_user.signup_link_invalid.app_error", nil, "", http.StatusBadRequest) } @@ -429,7 +428,7 @@ func (a *App) AddUserToTeamByToken(userId string, tokenId string) (*model.Team, close(uchan) }() - result = <-tchan + result := <-tchan if result.Err != nil { return nil, result.Err } @@ -1154,12 +1153,11 @@ func (a *App) GetTeamIdFromQuery(query url.Values) (string, *model.AppError) { inviteId := query.Get("id") if len(tokenId) > 0 { - result := <-a.Srv.Store.Token().GetByToken(tokenId) - if result.Err != nil { + token, err := a.Srv.Store.Token().GetByToken(tokenId) + if err != nil { return "", model.NewAppError("GetTeamIdFromQuery", "api.oauth.singup_with_oauth.invalid_link.app_error", nil, "", http.StatusBadRequest) } - token := result.Data.(*model.Token) if token.Type != TOKEN_TYPE_TEAM_INVITATION { return "", model.NewAppError("GetTeamIdFromQuery", "api.oauth.singup_with_oauth.invalid_link.app_error", nil, "", http.StatusBadRequest) } diff --git a/app/team_test.go b/app/team_test.go index 72945b1dfe..1d947681fa 100644 --- a/app/team_test.go +++ b/app/team_test.go @@ -259,9 +259,8 @@ func TestAddUserToTeamByToken(t *testing.T) { t.Log(err) t.Fatal("Should add user to the team") } - if result := <-th.App.Srv.Store.Token().GetByToken(token.Token); result.Err == nil { - t.Fatal("The token must be deleted after be used") - } + _, err := th.App.Srv.Store.Token().GetByToken(token.Token) + require.NotNil(t, err, "The token must be deleted after be used") }) t.Run("group-constrained team", func(t *testing.T) { diff --git a/app/user.go b/app/user.go index 2004b813f0..81eda4aa8c 100644 --- a/app/user.go +++ b/app/user.go @@ -50,12 +50,11 @@ func (a *App) CreateUserWithToken(user *model.User, tokenId string) (*model.User return nil, err } - result := <-a.Srv.Store.Token().GetByToken(tokenId) - if result.Err != nil { - return nil, model.NewAppError("CreateUserWithToken", "api.user.create_user.signup_link_invalid.app_error", nil, result.Err.Error(), http.StatusBadRequest) + token, err := a.Srv.Store.Token().GetByToken(tokenId) + if err != nil { + return nil, model.NewAppError("CreateUserWithToken", "api.user.create_user.signup_link_invalid.app_error", nil, err.Error(), http.StatusBadRequest) } - token := result.Data.(*model.Token) if token.Type != TOKEN_TYPE_TEAM_INVITATION { return nil, model.NewAppError("CreateUserWithToken", "api.user.create_user.signup_link_invalid.app_error", nil, "", http.StatusBadRequest) } @@ -1350,11 +1349,10 @@ func (a *App) CreatePasswordRecoveryToken(userId, email string) (*model.Token, * } func (a *App) GetPasswordRecoveryToken(token string) (*model.Token, *model.AppError) { - result := <-a.Srv.Store.Token().GetByToken(token) - if result.Err != nil { - return nil, model.NewAppError("GetPasswordRecoveryToken", "api.user.reset_password.invalid_link.app_error", nil, result.Err.Error(), http.StatusBadRequest) + rtoken, err := a.Srv.Store.Token().GetByToken(token) + if err != nil { + return nil, model.NewAppError("GetPasswordRecoveryToken", "api.user.reset_password.invalid_link.app_error", nil, err.Error(), http.StatusBadRequest) } - rtoken := result.Data.(*model.Token) if rtoken.Type != TOKEN_TYPE_PASSWORD_RECOVERY { return nil, model.NewAppError("GetPasswordRecoveryToken", "api.user.reset_password.broken_token.app_error", nil, "", http.StatusBadRequest) } @@ -1617,11 +1615,10 @@ func (a *App) CreateVerifyEmailToken(userId string, newEmail string) (*model.Tok } func (a *App) GetVerifyEmailToken(token string) (*model.Token, *model.AppError) { - result := <-a.Srv.Store.Token().GetByToken(token) - if result.Err != nil { - return nil, model.NewAppError("GetVerifyEmailToken", "api.user.verify_email.bad_link.app_error", nil, result.Err.Error(), http.StatusBadRequest) + rtoken, err := a.Srv.Store.Token().GetByToken(token) + if err != nil { + return nil, model.NewAppError("GetVerifyEmailToken", "api.user.verify_email.bad_link.app_error", nil, err.Error(), http.StatusBadRequest) } - rtoken := result.Data.(*model.Token) if rtoken.Type != TOKEN_TYPE_VERIFY_EMAIL { return nil, model.NewAppError("GetVerifyEmailToken", "api.user.verify_email.broken_token.app_error", nil, "", http.StatusBadRequest) } diff --git a/app/user_test.go b/app/user_test.go index 74997d0d11..024cbb182c 100644 --- a/app/user_test.go +++ b/app/user_test.go @@ -618,9 +618,8 @@ func TestCreateUserWithToken(t *testing.T) { if newUser.Email != invitationEmail { t.Fatal("The user email must be the invitation one") } - if result := <-th.App.Srv.Store.Token().GetByToken(token.Token); result.Err == nil { - t.Fatal("The token must be deleted after be used") - } + _, err = th.App.Srv.Store.Token().GetByToken(token.Token) + require.NotNil(t, err, "The token must be deleted after be used") }) } diff --git a/store/sqlstore/tokens_store.go b/store/sqlstore/tokens_store.go index 5810d67629..b55a7d7b13 100644 --- a/store/sqlstore/tokens_store.go +++ b/store/sqlstore/tokens_store.go @@ -52,20 +52,18 @@ func (s SqlTokenStore) Delete(token string) store.StoreChannel { }) } -func (s SqlTokenStore) GetByToken(tokenString string) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - token := model.Token{} +func (s SqlTokenStore) GetByToken(tokenString string) (*model.Token, *model.AppError) { + token := &model.Token{} - if err := s.GetReplica().SelectOne(&token, "SELECT * FROM Tokens WHERE Token = :Token", map[string]interface{}{"Token": tokenString}); err != nil { - if err == sql.ErrNoRows { - result.Err = model.NewAppError("SqlTokenStore.GetByToken", "store.sql_recover.get_by_code.app_error", nil, err.Error(), http.StatusBadRequest) - } else { - result.Err = model.NewAppError("SqlTokenStore.GetByToken", "store.sql_recover.get_by_code.app_error", nil, err.Error(), http.StatusInternalServerError) - } + if err := s.GetReplica().SelectOne(token, "SELECT * FROM Tokens WHERE Token = :Token", map[string]interface{}{"Token": tokenString}); err != nil { + if err == sql.ErrNoRows { + return nil, model.NewAppError("SqlTokenStore.GetByToken", "store.sql_recover.get_by_code.app_error", nil, err.Error(), http.StatusBadRequest) } - result.Data = &token - }) + return nil, model.NewAppError("SqlTokenStore.GetByToken", "store.sql_recover.get_by_code.app_error", nil, err.Error(), http.StatusInternalServerError) + } + + return token, nil } func (s SqlTokenStore) Cleanup() { diff --git a/store/store.go b/store/store.go index 58ee08517d..dfd64092e9 100644 --- a/store/store.go +++ b/store/store.go @@ -448,7 +448,7 @@ type LicenseStore interface { type TokenStore interface { Save(recovery *model.Token) StoreChannel Delete(token string) StoreChannel - GetByToken(token string) StoreChannel + GetByToken(token string) (*model.Token, *model.AppError) Cleanup() RemoveAllTokensByType(tokenType string) StoreChannel } diff --git a/store/storetest/mocks/TokenStore.go b/store/storetest/mocks/TokenStore.go index fb4c62406c..2205c78748 100644 --- a/store/storetest/mocks/TokenStore.go +++ b/store/storetest/mocks/TokenStore.go @@ -35,19 +35,28 @@ func (_m *TokenStore) Delete(token string) store.StoreChannel { } // GetByToken provides a mock function with given fields: token -func (_m *TokenStore) GetByToken(token string) store.StoreChannel { +func (_m *TokenStore) GetByToken(token string) (*model.Token, *model.AppError) { ret := _m.Called(token) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(string) store.StoreChannel); ok { + var r0 *model.Token + if rf, ok := ret.Get(0).(func(string) *model.Token); ok { r0 = rf(token) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.Token) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { + r1 = rf(token) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // RemoveAllTokensByType provides a mock function with given fields: tokenType