TokenStore migration to return plain errors (#14875)

* TokenStore migration to return plain errors

* Fix translations

* Fix: returned error is ignored and http.StatusBadRequest is always returned

* Fix

* Fix translations

* Suggestions

* Changed from BadRequest to NotFound

* Setting the correct http status

* Changed test to expect 404 status

* Fix error

api4/team_test.go

@@ -1826,8 +1826,8 @@ func TestAddTeamMember(t *testing.T) {
 
 	require.Equal(t, tm.TeamId, team.Id, "team ids should have matched")
 
-	_, err = th.App.Srv().Store.Token().GetByToken(token.Token)
+	_, nErr := th.App.Srv().Store.Token().GetByToken(token.Token)
-	require.NotNil(t, err, "The token must be deleted after be used")
+	require.NotNil(t, nErr, "The token must be deleted after be used")
 
 	tm, resp = Client.AddTeamMemberFromInvite("junk", "")
 	CheckBadRequestStatus(t, resp)

api4/user.go

@@ -107,10 +107,16 @@ func createUser(c *Context, w http.ResponseWriter, r *http.Request) {
 	var ruser *model.User
 	var err *model.AppError
 	if len(tokenId) > 0 {
-		var token *model.Token
+		token, nErr := c.App.Srv().Store.Token().GetByToken(tokenId)
-		token, err = c.App.Srv().Store.Token().GetByToken(tokenId)
+		if nErr != nil {
-		if err != nil {
+			var status int
-			c.Err = model.NewAppError("CreateUserWithToken", "api.user.create_user.signup_link_invalid.app_error", nil, err.Error(), http.StatusBadRequest)
+			switch nErr.(type) {
+			case *store.ErrNotFound:
+				status = http.StatusNotFound
+			default:
+				status = http.StatusInternalServerError
+			}
+			c.Err = model.NewAppError("CreateUserWithToken", "api.user.create_user.signup_link_invalid.app_error", nil, nErr.Error(), status)
 			return
 		}
 		auditRec.AddMeta("token_type", token.Type)

api4/user_test.go

@@ -256,7 +256,7 @@ func TestCreateUserWithToken(t *testing.T) {
 		user := model.User{Email: th.GenerateTestEmail(), Nickname: "Corey Hulen", Password: "hello1", Username: GenerateTestUsername(), Roles: model.SYSTEM_ADMIN_ROLE_ID + " " + model.SYSTEM_USER_ROLE_ID}
 
 		_, resp := th.Client.CreateUserWithToken(&user, "wrong")
-		CheckBadRequestStatus(t, resp)
+		CheckNotFoundStatus(t, resp)
 		CheckErrorMessage(t, resp, "api.user.create_user.signup_link_invalid.app_error")
 	})
 

app/email.go

@@ -571,8 +571,14 @@ func (es *EmailService) CreateVerifyEmailToken(userId string, newEmail string) (
 
 	token := model.NewToken(TOKEN_TYPE_VERIFY_EMAIL, string(jsonData))
 
-	if err := es.srv.Store.Token().Save(token); err != nil {
+	if err = es.srv.Store.Token().Save(token); err != nil {
-		return nil, err
+		var appErr *model.AppError
+		switch {
+		case errors.As(err, &appErr):
+			return nil, appErr
+		default:
+			return nil, model.NewAppError("CreateVerifyEmailToken", "app.recover.save.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
 	}
 
 	return token, nil

app/oauth.go

@@ -6,6 +6,7 @@ package app
 import (
 	"bytes"
 	b64 "encoding/base64"
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -596,7 +597,13 @@ func (a *App) CreateOAuthStateToken(extra string) (*model.Token, *model.AppError
 	token := model.NewToken(model.TOKEN_TYPE_OAUTH, extra)
 
 	if err := a.Srv().Store.Token().Save(token); err != nil {
-		return nil, err
+		var appErr *model.AppError
+		switch {
+		case errors.As(err, &appErr):
+			return nil, appErr
+		default:
+			return nil, model.NewAppError("CreateOAuthStateToken", "app.recover.save.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
 	}
 
 	return token, nil

app/team_test.go

@@ -256,8 +256,8 @@ func TestAddUserToTeamByToken(t *testing.T) {
 		_, err := th.App.AddUserToTeamByToken(ruser.Id, token.Token)
 		require.Nil(t, err, "Should add user to the team")
 
-		_, err = th.App.Srv().Store.Token().GetByToken(token.Token)
+		_, nErr := th.App.Srv().Store.Token().GetByToken(token.Token)
-		require.NotNil(t, err, "The token must be deleted after be used")
+		require.NotNil(t, nErr, "The token must be deleted after be used")
 
 		members, err := th.App.GetChannelMembersForUser(th.BasicTeam.Id, ruser.Id)
 		require.Nil(t, err)
@@ -356,8 +356,8 @@ func TestAddUserToTeamByToken(t *testing.T) {
 		_, err := th.App.AddUserToTeamByToken(rguest.Id, token.Token)
 		require.Nil(t, err, "Should add user to the team")
 
-		_, err = th.App.Srv().Store.Token().GetByToken(token.Token)
+		_, nErr := th.App.Srv().Store.Token().GetByToken(token.Token)
-		require.NotNil(t, err, "The token must be deleted after be used")
+		require.NotNil(t, nErr, "The token must be deleted after be used")
 
 		members, err := th.App.GetChannelMembersForUser(th.BasicTeam.Id, rguest.Id)
 		require.Nil(t, err)

app/user.go

@@ -1361,7 +1361,13 @@ func (a *App) CreatePasswordRecoveryToken(userId, email string) (*model.Token, *
 	token := model.NewToken(TOKEN_TYPE_PASSWORD_RECOVERY, string(jsonData))
 
 	if err := a.Srv().Store.Token().Save(token); err != nil {
-		return nil, err
+		var appErr *model.AppError
+		switch {
+		case errors.As(err, &appErr):
+			return nil, appErr
+		default:
+			return nil, model.NewAppError("CreatePasswordRecoveryToken", "app.recover.save.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
 	}
 
 	return token, nil
@@ -1379,7 +1385,11 @@ func (a *App) GetPasswordRecoveryToken(token string) (*model.Token, *model.AppEr
 }
 
 func (a *App) DeleteToken(token *model.Token) *model.AppError {
-	return a.Srv().Store.Token().Delete(token.Token)
+	err := a.Srv().Store.Token().Delete(token.Token)
+	if err != nil {
+		return model.NewAppError("DeleteToken", "app.recover.delete.app_error", nil, err.Error(), http.StatusInternalServerError)
+	}
+	return nil
 }
 
 func (a *App) UpdateUserRoles(userId string, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError) {

app/user_test.go

@@ -678,8 +678,8 @@ func TestCreateUserWithToken(t *testing.T) {
 		assert.False(t, newUser.IsGuest())
 		require.Equal(t, invitationEmail, newUser.Email, "The user email must be the invitation one")
 
-		_, err = th.App.Srv().Store.Token().GetByToken(token.Token)
+		_, nErr := th.App.Srv().Store.Token().GetByToken(token.Token)
-		require.NotNil(t, err, "The token must be deleted after be used")
+		require.NotNil(t, nErr, "The token must be deleted after be used")
 
 		members, err := th.App.GetChannelMembersForUser(th.BasicTeam.Id, newUser.Id)
 		require.Nil(t, err)
@@ -699,8 +699,8 @@ func TestCreateUserWithToken(t *testing.T) {
 
 		assert.True(t, newGuest.IsGuest())
 		require.Equal(t, invitationEmail, newGuest.Email, "The user email must be the invitation one")
-		_, err = th.App.Srv().Store.Token().GetByToken(token.Token)
+		_, nErr := th.App.Srv().Store.Token().GetByToken(token.Token)
-		require.NotNil(t, err, "The token must be deleted after be used")
+		require.NotNil(t, nErr, "The token must be deleted after be used")
 
 		members, err := th.App.GetChannelMembersForUser(th.BasicTeam.Id, newGuest.Id)
 		require.Nil(t, err)
@@ -744,8 +744,8 @@ func TestCreateUserWithToken(t *testing.T) {
 		require.Nil(t, err)
 		assert.True(t, newGuest.IsGuest())
 		require.Equal(t, grantedInvitationEmail, newGuest.Email)
-		_, err = th.App.Srv().Store.Token().GetByToken(grantedDomainToken.Token)
+		_, nErr := th.App.Srv().Store.Token().GetByToken(grantedDomainToken.Token)
-		require.NotNil(t, err)
+		require.NotNil(t, nErr)
 
 		members, err := th.App.GetChannelMembersForUser(th.BasicTeam.Id, newGuest.Id)
 		require.Nil(t, err)
@@ -781,8 +781,8 @@ func TestCreateUserWithToken(t *testing.T) {
 		require.Nil(t, err)
 		assert.True(t, newGuest.IsGuest())
 		assert.Equal(t, invitationEmail, newGuest.Email, "The user email must be the invitation one")
-		_, err = th.App.Srv().Store.Token().GetByToken(token.Token)
+		_, nErr := th.App.Srv().Store.Token().GetByToken(token.Token)
-		require.NotNil(t, err)
+		require.NotNil(t, nErr)
 
 		members, err := th.App.GetChannelMembersForUser(th.BasicTeam.Id, newGuest.Id)
 		require.Nil(t, err)

i18n/en.json

@@ -3982,6 +3982,14 @@
     "id": "app.reaction.save.save.app_error",
     "translation": "Unable to save reaction."
   },
+  {
+    "id": "app.recover.delete.app_error",
+    "translation": "Unable to delete token."
+  },
+  {
+    "id": "app.recover.save.app_error",
+    "translation": "Unable to save the token."
+  },
   {
     "id": "app.role.check_roles_exist.role_not_found",
     "translation": "The provided role does not exist"
@@ -6966,22 +6974,6 @@
     "id": "store.sql_preference.update.app_error",
     "translation": "Unable to update the preference."
   },
-  {
-    "id": "store.sql_recover.delete.app_error",
-    "translation": "Unable to delete token."
-  },
-  {
-    "id": "store.sql_recover.get_by_code.app_error",
-    "translation": "Unable to get a token with this code."
-  },
-  {
-    "id": "store.sql_recover.remove_all_tokens_by_type.app_error",
-    "translation": "Unable to remove all the tokens of a type."
-  },
-  {
-    "id": "store.sql_recover.save.app_error",
-    "translation": "Unable to save the token."
-  },
   {
     "id": "store.sql_role.delete.update.app_error",
     "translation": "Unable to delete the role."

store/opentracing_layer.go

@@ -7486,7 +7486,7 @@ func (s *OpenTracingLayerTokenStore) Cleanup() {
 
 }
 
-func (s *OpenTracingLayerTokenStore) Delete(token string) *model.AppError {
+func (s *OpenTracingLayerTokenStore) Delete(token string) error {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "TokenStore.Delete")
 	s.Root.Store.SetContext(newCtx)
@@ -7504,7 +7504,7 @@ func (s *OpenTracingLayerTokenStore) Delete(token string) *model.AppError {
 	return resultVar0
 }
 
-func (s *OpenTracingLayerTokenStore) GetByToken(token string) (*model.Token, *model.AppError) {
+func (s *OpenTracingLayerTokenStore) GetByToken(token string) (*model.Token, error) {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "TokenStore.GetByToken")
 	s.Root.Store.SetContext(newCtx)
@@ -7522,7 +7522,7 @@ func (s *OpenTracingLayerTokenStore) GetByToken(token string) (*model.Token, *mo
 	return resultVar0, resultVar1
 }
 
-func (s *OpenTracingLayerTokenStore) RemoveAllTokensByType(tokenType string) *model.AppError {
+func (s *OpenTracingLayerTokenStore) RemoveAllTokensByType(tokenType string) error {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "TokenStore.RemoveAllTokensByType")
 	s.Root.Store.SetContext(newCtx)
@@ -7540,7 +7540,7 @@ func (s *OpenTracingLayerTokenStore) RemoveAllTokensByType(tokenType string) *mo
 	return resultVar0
 }
 
-func (s *OpenTracingLayerTokenStore) Save(recovery *model.Token) *model.AppError {
+func (s *OpenTracingLayerTokenStore) Save(recovery *model.Token) error {
 	origCtx := s.Root.Store.Context()
 	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "TokenStore.Save")
 	s.Root.Store.SetContext(newCtx)

store/sqlstore/tokens_store.go

@@ -5,11 +5,13 @@ package sqlstore
 
 import (
 	"database/sql"
-	"net/http"
+	"fmt"
 
 	"github.com/mattermost/mattermost-server/v5/mlog"
 	"github.com/mattermost/mattermost-server/v5/model"
 	"github.com/mattermost/mattermost-server/v5/store"
+
+	"github.com/pkg/errors"
 )
 
 type SqlTokenStore struct {
@@ -32,33 +34,33 @@ func newSqlTokenStore(sqlStore SqlStore) store.TokenStore {
 func (s SqlTokenStore) createIndexesIfNotExists() {
 }
 
-func (s SqlTokenStore) Save(token *model.Token) *model.AppError {
+func (s SqlTokenStore) Save(token *model.Token) error {
 	if err := token.IsValid(); err != nil {
 		return err
 	}
 
 	if err := s.GetMaster().Insert(token); err != nil {
-		return model.NewAppError("SqlTokenStore.Save", "store.sql_recover.save.app_error", nil, "", http.StatusInternalServerError)
+		return errors.Wrap(err, "failed to save Token")
 	}
 	return nil
 }
 
-func (s SqlTokenStore) Delete(token string) *model.AppError {
+func (s SqlTokenStore) Delete(token string) error {
 	if _, err := s.GetMaster().Exec("DELETE FROM Tokens WHERE Token = :Token", map[string]interface{}{"Token": token}); err != nil {
-		return model.NewAppError("SqlTokenStore.Delete", "store.sql_recover.delete.app_error", nil, "", http.StatusInternalServerError)
+		return errors.Wrapf(err, "failed to delete Token with value %s", token)
 	}
 	return nil
 }
 
-func (s SqlTokenStore) GetByToken(tokenString string) (*model.Token, *model.AppError) {
+func (s SqlTokenStore) GetByToken(tokenString string) (*model.Token, error) {
 	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 {
-			return nil, model.NewAppError("SqlTokenStore.GetByToken", "store.sql_recover.get_by_code.app_error", nil, err.Error(), http.StatusBadRequest)
+			return nil, store.NewErrNotFound("Token", fmt.Sprintf("Token=%s", tokenString))
 		}
 
-		return nil, model.NewAppError("SqlTokenStore.GetByToken", "store.sql_recover.get_by_code.app_error", nil, err.Error(), http.StatusInternalServerError)
+		return nil, errors.Wrapf(err, "failed to get Token with value %s", tokenString)
 	}
 
 	return token, nil
@@ -72,9 +74,9 @@ func (s SqlTokenStore) Cleanup() {
 	}
 }
 
-func (s SqlTokenStore) RemoveAllTokensByType(tokenType string) *model.AppError {
+func (s SqlTokenStore) RemoveAllTokensByType(tokenType string) error {
 	if _, err := s.GetMaster().Exec("DELETE FROM Tokens WHERE Type = :TokenType", map[string]interface{}{"TokenType": tokenType}); err != nil {
-		return model.NewAppError("SqlTokenStore.RemoveAllTokensByType", "store.sql_recover.remove_all_tokens_by_type.app_error", nil, err.Error(), http.StatusInternalServerError)
+		return errors.Wrapf(err, "failed to remove all Tokens with Type=%s", tokenType)
 	}
 	return nil
 }

store/store.go

@@ -512,11 +512,11 @@ type LicenseStore interface {
 }
 
 type TokenStore interface {
-	Save(recovery *model.Token) *model.AppError
+	Save(recovery *model.Token) error
-	Delete(token string) *model.AppError
+	Delete(token string) error
-	GetByToken(token string) (*model.Token, *model.AppError)
+	GetByToken(token string) (*model.Token, error)
 	Cleanup()
-	RemoveAllTokensByType(tokenType string) *model.AppError
+	RemoveAllTokensByType(tokenType string) error
 }
 
 type EmojiStore interface {

store/storetest/mocks/TokenStore.go

@@ -20,23 +20,21 @@ func (_m *TokenStore) Cleanup() {
 }
 
 // Delete provides a mock function with given fields: token
-func (_m *TokenStore) Delete(token string) *model.AppError {
+func (_m *TokenStore) Delete(token string) error {
 	ret := _m.Called(token)
 
-	var r0 *model.AppError
+	var r0 error
-	if rf, ok := ret.Get(0).(func(string) *model.AppError); ok {
+	if rf, ok := ret.Get(0).(func(string) error); ok {
 		r0 = rf(token)
 	} else {
-		if ret.Get(0) != nil {
+		r0 = ret.Error(0)
-			r0 = ret.Get(0).(*model.AppError)
-		}
 	}
 
 	return r0
 }
 
 // GetByToken provides a mock function with given fields: token
-func (_m *TokenStore) GetByToken(token string) (*model.Token, *model.AppError) {
+func (_m *TokenStore) GetByToken(token string) (*model.Token, error) {
 	ret := _m.Called(token)
 
 	var r0 *model.Token
@@ -48,45 +46,39 @@ func (_m *TokenStore) GetByToken(token string) (*model.Token, *model.AppError) {
 		}
 	}
 
-	var r1 *model.AppError
+	var r1 error
-	if rf, ok := ret.Get(1).(func(string) *model.AppError); ok {
+	if rf, ok := ret.Get(1).(func(string) error); ok {
 		r1 = rf(token)
 	} else {
-		if ret.Get(1) != nil {
+		r1 = ret.Error(1)
-			r1 = ret.Get(1).(*model.AppError)
-		}
 	}
 
 	return r0, r1
 }
 
 // RemoveAllTokensByType provides a mock function with given fields: tokenType
-func (_m *TokenStore) RemoveAllTokensByType(tokenType string) *model.AppError {
+func (_m *TokenStore) RemoveAllTokensByType(tokenType string) error {
 	ret := _m.Called(tokenType)
 
-	var r0 *model.AppError
+	var r0 error
-	if rf, ok := ret.Get(0).(func(string) *model.AppError); ok {
+	if rf, ok := ret.Get(0).(func(string) error); ok {
 		r0 = rf(tokenType)
 	} else {
-		if ret.Get(0) != nil {
+		r0 = ret.Error(0)
-			r0 = ret.Get(0).(*model.AppError)
-		}
 	}
 
 	return r0
 }
 
 // Save provides a mock function with given fields: recovery
-func (_m *TokenStore) Save(recovery *model.Token) *model.AppError {
+func (_m *TokenStore) Save(recovery *model.Token) error {
 	ret := _m.Called(recovery)
 
-	var r0 *model.AppError
+	var r0 error
-	if rf, ok := ret.Get(0).(func(*model.Token) *model.AppError); ok {
+	if rf, ok := ret.Get(0).(func(*model.Token) error); ok {
 		r0 = rf(recovery)
 	} else {
-		if ret.Get(0) != nil {
+		r0 = ret.Error(0)
-			r0 = ret.Get(0).(*model.AppError)
-		}
 	}
 
 	return r0

store/timer_layer.go

@@ -6760,7 +6760,7 @@ func (s *TimerLayerTokenStore) Cleanup() {
 	}
 }
 
-func (s *TimerLayerTokenStore) Delete(token string) *model.AppError {
+func (s *TimerLayerTokenStore) Delete(token string) error {
 	start := timemodule.Now()
 
 	resultVar0 := s.TokenStore.Delete(token)
@@ -6776,7 +6776,7 @@ func (s *TimerLayerTokenStore) Delete(token string) *model.AppError {
 	return resultVar0
 }
 
-func (s *TimerLayerTokenStore) GetByToken(token string) (*model.Token, *model.AppError) {
+func (s *TimerLayerTokenStore) GetByToken(token string) (*model.Token, error) {
 	start := timemodule.Now()
 
 	resultVar0, resultVar1 := s.TokenStore.GetByToken(token)
@@ -6792,7 +6792,7 @@ func (s *TimerLayerTokenStore) GetByToken(token string) (*model.Token, *model.Ap
 	return resultVar0, resultVar1
 }
 
-func (s *TimerLayerTokenStore) RemoveAllTokensByType(tokenType string) *model.AppError {
+func (s *TimerLayerTokenStore) RemoveAllTokensByType(tokenType string) error {
 	start := timemodule.Now()
 
 	resultVar0 := s.TokenStore.RemoveAllTokensByType(tokenType)
@@ -6808,7 +6808,7 @@ func (s *TimerLayerTokenStore) RemoveAllTokensByType(tokenType string) *model.Ap
 	return resultVar0
 }
 
-func (s *TimerLayerTokenStore) Save(recovery *model.Token) *model.AppError {
+func (s *TimerLayerTokenStore) Save(recovery *model.Token) error {
 	start := timemodule.Now()
 
 	resultVar0 := s.TokenStore.Save(recovery)