From 7664eab32d8434d818915776c50abe4d36263170 Mon Sep 17 00:00:00 2001 From: dv8silencer <15720668+dv8silencer@users.noreply.github.com> Date: Wed, 30 Sep 2020 09:13:37 -0500 Subject: [PATCH] Introduce checks for existing wallets into two edge cases (#7349) * Add check for preexisting wallet * Reminder * Create another check. Adjust tests * Wording in comment * Remove unnecessary logic. Add another regression test. * Spacing * Prepare to merge master * Refactor wallet checks * Fixed test * Revert back to original location of check * Fix test * Fix test * Address linter feedback * Align CreateWallet() with recent changes * Align RecoverWallet with recent changes * Correct test error message * Refactor test to minimize duplication * rename to isValid and adjust function code * refactor * Improve IsValid() * Fix tests * Remove comment * Fix documentation of IsValid() * Align behavior of OpenWalletOrElseCli * Fix tests * Realigning error msg with prior behavior * Fix another test * Correct logic * small change in logic * Fix err messages * Create consts for repeated strings * Add comments to new constants * gofmt * Adjust WalletConfig behavior so it is closer to prior-to-PR. Adjust test accordingly * adjust error messages Co-authored-by: dv8silencer <15720668+dv8silencer@users.noreply.github.com> --- validator/accounts/v2/wallet/wallet.go | 137 +++++++++++-------- validator/accounts/v2/wallet/wallet_test.go | 47 +++++-- validator/accounts/v2/wallet_create.go | 9 +- validator/accounts/v2/wallet_recover.go | 14 +- validator/accounts/v2/wallet_recover_test.go | 108 +++++++-------- validator/rpc/auth.go | 22 ++- validator/rpc/wallet.go | 71 ++++++++-- validator/rpc/wallet_test.go | 25 +++- 8 files changed, 283 insertions(+), 150 deletions(-) diff --git a/validator/accounts/v2/wallet/wallet.go b/validator/accounts/v2/wallet/wallet.go index 7f9c81255a..5aef7db581 100644 --- a/validator/accounts/v2/wallet/wallet.go +++ b/validator/accounts/v2/wallet/wallet.go @@ -42,6 +42,12 @@ const ( // ConfirmPasswordPromptText for confirming a wallet password. ConfirmPasswordPromptText = "Confirm password" hashCost = 8 + // CheckExistsErrMsg for when there is an error while checking for a wallet + CheckExistsErrMsg = "could not check if wallet exists" + // CheckValidityErrMsg for when there is an error while checking wallet validity + CheckValidityErrMsg = "could not check if wallet is valid" + // InvalidWalletErrMsg for when a directory does not contain a valid wallet + InvalidWalletErrMsg = "directory does not contain valid wallet" ) var ( @@ -100,35 +106,73 @@ func New(cfg *Config) *Wallet { } } -// Exists check if a wallet at the specified directory -// exists and has valid information in it. -func Exists(walletDir string) error { +// Exists checks if directory at walletDir exists +func Exists(walletDir string) (bool, error) { dirExists, err := fileutil.HasDir(walletDir) if err != nil { - return errors.Wrap(err, "could not parse wallet directory") + return false, errors.Wrap(err, "could not parse wallet directory") } - if dirExists { - isEmptyWallet, err := isEmptyWallet(walletDir) - if err != nil { - return errors.Wrap(err, "could not check if wallet has files") - } - if isEmptyWallet { - return ErrNoWalletFound - } - return nil + return dirExists, nil +} + +// IsValid checks if a folder contains a single key directory such as `derived`, `remote` or `direct`. +// Returns true if one of those subdirectories exist, false otherwise. +func IsValid(walletDir string) (bool, error) { + expanded, err := fileutil.ExpandPath(walletDir) + if err != nil { + return false, err } - return ErrNoWalletFound + f, err := os.Open(expanded) + if err != nil { + return false, err + } + defer func() { + if err := f.Close(); err != nil { + log.Debugf("Could not close directory: %s", expanded) + } + }() + names, err := f.Readdirnames(-1) + if err != nil { + return false, err + } + + if len(names) == 0 { + return false, ErrNoWalletFound + } + + // Count how many wallet types we have in the directory + numWalletTypes := 0 + for _, name := range names { + // Nil error means input name is `derived`, `remote` or `direct` + _, err = v2keymanager.ParseKind(name) + if err == nil { + numWalletTypes++ + } + } + return numWalletTypes == 1, nil } // OpenWalletOrElseCli tries to open the wallet and if it fails or no wallet // is found, invokes a callback function. func OpenWalletOrElseCli(cliCtx *cli.Context, otherwise func(cliCtx *cli.Context) (*Wallet, error)) (*Wallet, error) { - if err := Exists(cliCtx.String(flags.WalletDirFlag.Name)); err != nil { - if errors.Is(err, ErrNoWalletFound) { - return otherwise(cliCtx) - } - return nil, errors.Wrap(err, "could not check if wallet exists") + exists, err := Exists(cliCtx.String(flags.WalletDirFlag.Name)) + if err != nil { + return nil, errors.Wrap(err, CheckExistsErrMsg) } + if !exists { + return otherwise(cliCtx) + } + isValid, err := IsValid(cliCtx.String(flags.WalletDirFlag.Name)) + if err == ErrNoWalletFound { + return otherwise(cliCtx) + } + if err != nil { + return nil, errors.Wrap(err, CheckValidityErrMsg) + } + if !isValid { + return nil, errors.New(InvalidWalletErrMsg) + } + walletDir, err := prompt.InputDirectory(cliCtx, prompt.WalletDirPromptText, flags.WalletDirFlag) if err != nil { return nil, err @@ -160,12 +204,25 @@ func OpenWalletOrElseCli(cliCtx *cli.Context, otherwise func(cliCtx *cli.Context // type of keymanager associated with the wallet by reading files in the wallet // path, if applicable. If a wallet does not exist, returns an appropriate error. func OpenWallet(ctx context.Context, cfg *Config) (*Wallet, error) { - if err := Exists(cfg.WalletDir); err != nil { - if errors.Is(err, ErrNoWalletFound) { - return nil, ErrNoWalletFound - } - return nil, errors.Wrap(err, "could not check if wallet exists") + exists, err := Exists(cfg.WalletDir) + if err != nil { + return nil, errors.Wrap(err, CheckExistsErrMsg) } + if !exists { + return nil, ErrNoWalletFound + } + valid, err := IsValid(cfg.WalletDir) + // ErrNoWalletFound represents both a directory that does not exist as well as an empty directory + if err == ErrNoWalletFound { + return nil, ErrNoWalletFound + } + if err != nil { + return nil, errors.Wrap(err, CheckValidityErrMsg) + } + if !valid { + return nil, errors.New(InvalidWalletErrMsg) + } + keymanagerKind, err := readKeymanagerKindFromWalletPath(cfg.WalletDir) if err != nil { return nil, errors.Wrap(err, "could not read keymanager kind for wallet") @@ -445,38 +502,6 @@ func readKeymanagerKindFromWalletPath(walletPath string) (v2keymanager.Kind, err return 0, errors.New("no keymanager folder, 'direct', 'remote', nor 'derived' found in wallet path") } -// isEmptyWallet checks if a folder consists key directory such as `derived`, `remote` or `direct`. -// Returns true if exists, false otherwise. -func isEmptyWallet(name string) (bool, error) { - expanded, err := fileutil.ExpandPath(name) - if err != nil { - return false, err - } - f, err := os.Open(expanded) - if err != nil { - return false, err - } - defer func() { - if err := f.Close(); err != nil { - log.Debugf("Could not close directory: %s", expanded) - } - }() - names, err := f.Readdirnames(-1) - if err == io.EOF { - return true, nil - } - - for _, n := range names { - // Nil error means input name is `derived`, `remote` or `direct`, the wallet is not empty. - _, err := v2keymanager.ParseKind(n) - if err == nil { - return false, nil - } - } - - return true, err -} - func inputPassword( cliCtx *cli.Context, passwordFileFlag *cli.StringFlag, diff --git a/validator/accounts/v2/wallet/wallet_test.go b/validator/accounts/v2/wallet/wallet_test.go index 253b006205..006be70c17 100644 --- a/validator/accounts/v2/wallet/wallet_test.go +++ b/validator/accounts/v2/wallet/wallet_test.go @@ -110,18 +110,45 @@ func setupWalletAndPasswordsDir(t testing.TB) (string, string, string) { return walletDir, passwordsDir, passwordFilePath } -func Test_IsEmptyWallet_RandomFiles(t *testing.T) { - path := testutil.TempDir() - walletDir := filepath.Join(path, "test") - require.NoError(t, os.MkdirAll(walletDir, params.BeaconIoConfig().ReadWriteExecutePermissions), "Failed to remove directory") - err := wallet.Exists(path) - require.ErrorContains(t, "no wallet found at path", err) +func Test_Exists_RandomFiles(t *testing.T) { + randPath, err := rand.Int(rand.Reader, big.NewInt(1000000)) + require.NoError(t, err, "Could not generate random file path") + path := filepath.Join(testutil.TempDir(), fmt.Sprintf("/%d", randPath), "wallet") - walletDir = filepath.Join(path, "direct") - require.NoError(t, os.MkdirAll(walletDir, params.BeaconIoConfig().ReadWriteExecutePermissions), "Failed to remove directory") - err = wallet.Exists(path) + exists, err := wallet.Exists(path) + require.Equal(t, false, exists) require.NoError(t, err) - require.NoError(t, os.RemoveAll(walletDir), "Failed to remove directory") + + require.NoError(t, os.MkdirAll(path, params.BeaconIoConfig().ReadWriteExecutePermissions), "Failed to create directory") + t.Cleanup(func() { + require.NoError(t, os.RemoveAll(path), "Failed to remove directory") + }) + + exists, err = wallet.Exists(path) + require.NoError(t, err) + require.Equal(t, true, exists) +} + +func Test_IsValid_RandomFiles(t *testing.T) { + randPath, err := rand.Int(rand.Reader, big.NewInt(1000000)) + require.NoError(t, err, "Could not generate random file path") + path := filepath.Join(testutil.TempDir(), fmt.Sprintf("/%d", randPath), "wallet") + + require.NoError(t, os.MkdirAll(path, params.BeaconIoConfig().ReadWriteExecutePermissions), "Failed to create directory") + t.Cleanup(func() { + require.NoError(t, os.RemoveAll(path), "Failed to remove directory") + }) + + valid, err := wallet.IsValid(path) + require.ErrorContains(t, "no wallet found at path", err) + require.Equal(t, false, valid) + + walletDir := filepath.Join(path, "direct") + require.NoError(t, os.MkdirAll(walletDir, params.BeaconIoConfig().ReadWriteExecutePermissions), "Failed to create directory") + + valid, err = wallet.IsValid(path) + require.NoError(t, err) + require.Equal(t, true, valid) } func Test_LockUnlockFile(t *testing.T) { diff --git a/validator/accounts/v2/wallet_create.go b/validator/accounts/v2/wallet_create.go index 53dbba4fd8..539e28e8b5 100644 --- a/validator/accounts/v2/wallet_create.go +++ b/validator/accounts/v2/wallet_create.go @@ -6,7 +6,6 @@ import ( "github.com/manifoldco/promptui" "github.com/pkg/errors" - "github.com/prysmaticlabs/prysm/shared/fileutil" "github.com/prysmaticlabs/prysm/shared/promptutil" "github.com/prysmaticlabs/prysm/validator/accounts/v2/prompt" "github.com/prysmaticlabs/prysm/validator/accounts/v2/wallet" @@ -39,7 +38,7 @@ func CreateAndSaveWalletCli(cliCtx *cli.Context) (*wallet.Wallet, error) { } dir := createWalletConfig.WalletCfg.WalletDir - dirExists, err := fileutil.HasDir(dir) + dirExists, err := wallet.Exists(dir) if err != nil { return nil, err } @@ -47,6 +46,7 @@ func CreateAndSaveWalletCli(cliCtx *cli.Context) (*wallet.Wallet, error) { return nil, errors.New("a wallet already exists at this location. Please input an" + " alternative location for the new wallet or remove the current wallet") } + w, err := CreateWalletWithKeymanager(cliCtx.Context, createWalletConfig) if err != nil { return nil, errors.Wrap(err, "could not create wallet with keymanager") @@ -60,11 +60,6 @@ func CreateAndSaveWalletCli(cliCtx *cli.Context) (*wallet.Wallet, error) { // CreateWalletWithKeymanager specified by configuration options. func CreateWalletWithKeymanager(ctx context.Context, cfg *CreateWalletConfig) (*wallet.Wallet, error) { - if err := wallet.Exists(cfg.WalletCfg.WalletDir); err != nil { - if !errors.Is(err, wallet.ErrNoWalletFound) { - return nil, errors.Wrap(err, "could not check if wallet exists") - } - } w := wallet.New(&wallet.Config{ WalletDir: cfg.WalletCfg.WalletDir, KeymanagerKind: cfg.WalletCfg.KeymanagerKind, diff --git a/validator/accounts/v2/wallet_recover.go b/validator/accounts/v2/wallet_recover.go index 255d0dd235..cd466d453c 100644 --- a/validator/accounts/v2/wallet_recover.go +++ b/validator/accounts/v2/wallet_recover.go @@ -53,11 +53,6 @@ func RecoverWalletCli(cliCtx *cli.Context) error { if err != nil { return err } - if err := wallet.Exists(walletDir); err != nil { - if !errors.Is(err, wallet.ErrNoWalletFound) { - return errors.Wrap(err, "could not check if wallet exists") - } - } numAccounts, err := inputNumAccounts(cliCtx) if err != nil { return errors.Wrap(err, "could not get number of accounts to recover") @@ -84,6 +79,15 @@ func RecoverWalletCli(cliCtx *cli.Context) error { // RecoverWallet uses a menmonic seed phrase to recover a wallet into the path provided. func RecoverWallet(ctx context.Context, cfg *RecoverWalletConfig) (*wallet.Wallet, error) { + // Ensure that the wallet directory does not contain a wallet already + dirExists, err := wallet.Exists(cfg.WalletDir) + if err != nil { + return nil, err + } + if dirExists { + return nil, errors.New("a wallet already exists at this location. Please input an" + + " alternative location for the new wallet or remove the current wallet") + } w := wallet.New(&wallet.Config{ WalletDir: cfg.WalletDir, KeymanagerKind: v2keymanager.Derived, diff --git a/validator/accounts/v2/wallet_recover_test.go b/validator/accounts/v2/wallet_recover_test.go index f7eb9d4c1e..46cad9bf97 100644 --- a/validator/accounts/v2/wallet_recover_test.go +++ b/validator/accounts/v2/wallet_recover_test.go @@ -19,56 +19,70 @@ import ( "github.com/urfave/cli/v2" ) -func TestRecoverDerivedWallet(t *testing.T) { +type recoverCfgStruct struct { + walletDir string + passwordFilePath string + mnemonicFilePath string + numAccounts int64 +} + +func setupRecoverCfg(t *testing.T) *recoverCfgStruct { testDir := testutil.TempDir() walletDir := filepath.Join(testDir, walletDirName) - passwordsDir := filepath.Join(testDir, passwordDirName) - exportDir := filepath.Join(testDir, exportDirName) - defer func() { - assert.NoError(t, os.RemoveAll(walletDir)) - assert.NoError(t, os.RemoveAll(passwordsDir)) - assert.NoError(t, os.RemoveAll(exportDir)) - }() - passwordFilePath := filepath.Join(testDir, passwordFileName) require.NoError(t, ioutil.WriteFile(passwordFilePath, []byte(password), os.ModePerm)) mnemonicFilePath := filepath.Join(testDir, mnemonicFileName) require.NoError(t, ioutil.WriteFile(mnemonicFilePath, []byte(mnemonic), os.ModePerm)) - numAccounts := int64(4) + t.Cleanup(func() { + assert.NoError(t, os.RemoveAll(walletDir)) + assert.NoError(t, os.Remove(passwordFilePath)) + assert.NoError(t, os.Remove(mnemonicFilePath)) + }) + + return &recoverCfgStruct{ + walletDir: walletDir, + passwordFilePath: passwordFilePath, + mnemonicFilePath: mnemonicFilePath, + } +} + +func createRecoverCliCtx(t *testing.T, cfg *recoverCfgStruct) *cli.Context { app := cli.App{} set := flag.NewFlagSet("test", 0) - set.String(flags.WalletDirFlag.Name, walletDir, "") - set.String(flags.DeprecatedPasswordsDirFlag.Name, passwordsDir, "") - set.String(flags.WalletPasswordFileFlag.Name, passwordFilePath, "") + set.String(flags.WalletDirFlag.Name, cfg.walletDir, "") + set.String(flags.WalletPasswordFileFlag.Name, cfg.passwordFilePath, "") set.String(flags.KeymanagerKindFlag.Name, v2keymanager.Derived.String(), "") - set.String(flags.MnemonicFileFlag.Name, mnemonicFilePath, "") - set.Int64(flags.NumAccountsFlag.Name, numAccounts, "") - assert.NoError(t, set.Set(flags.WalletDirFlag.Name, walletDir)) - assert.NoError(t, set.Set(flags.DeprecatedPasswordsDirFlag.Name, passwordsDir)) - assert.NoError(t, set.Set(flags.WalletPasswordFileFlag.Name, passwordFilePath)) + set.String(flags.MnemonicFileFlag.Name, cfg.mnemonicFilePath, "") + set.Int64(flags.NumAccountsFlag.Name, cfg.numAccounts, "") + assert.NoError(t, set.Set(flags.WalletDirFlag.Name, cfg.walletDir)) + assert.NoError(t, set.Set(flags.WalletPasswordFileFlag.Name, cfg.passwordFilePath)) assert.NoError(t, set.Set(flags.KeymanagerKindFlag.Name, v2keymanager.Derived.String())) - assert.NoError(t, set.Set(flags.MnemonicFileFlag.Name, mnemonicFilePath)) - assert.NoError(t, set.Set(flags.NumAccountsFlag.Name, strconv.Itoa(int(numAccounts)))) - cliCtx := cli.NewContext(&app, set, nil) + assert.NoError(t, set.Set(flags.MnemonicFileFlag.Name, cfg.mnemonicFilePath)) + assert.NoError(t, set.Set(flags.NumAccountsFlag.Name, strconv.Itoa(int(cfg.numAccounts)))) + return cli.NewContext(&app, set, nil) +} +func TestRecoverDerivedWallet(t *testing.T) { + cfg := setupRecoverCfg(t) + cfg.numAccounts = 4 + cliCtx := createRecoverCliCtx(t, cfg) require.NoError(t, RecoverWalletCli(cliCtx)) ctx := context.Background() w, err := wallet.OpenWallet(cliCtx.Context, &wallet.Config{ - WalletDir: walletDir, + WalletDir: cfg.walletDir, WalletPassword: password, }) assert.NoError(t, err) encoded, err := w.ReadKeymanagerConfigFromDisk(ctx) assert.NoError(t, err) - cfg, err := derived.UnmarshalOptionsFile(encoded) + walletCfg, err := derived.UnmarshalOptionsFile(encoded) assert.NoError(t, err) - // We assert the created configuration was as desired. wantCfg := derived.DefaultKeymanagerOpts() - assert.DeepEqual(t, wantCfg, cfg) + assert.DeepEqual(t, wantCfg, walletCfg) keymanager, err := w.InitializeKeymanager(cliCtx.Context, true) require.NoError(t, err) @@ -78,43 +92,29 @@ func TestRecoverDerivedWallet(t *testing.T) { } names, err := km.ValidatingAccountNames(ctx) assert.NoError(t, err) - require.Equal(t, len(names), int(numAccounts)) - + require.Equal(t, len(names), int(cfg.numAccounts)) } // TestRecoverDerivedWallet_OneAccount is a test for regression in cases where the number of accounts recovered is 1 func TestRecoverDerivedWallet_OneAccount(t *testing.T) { - testDir := testutil.TempDir() - walletDir := filepath.Join(testDir, walletDirName) - defer func() { - assert.NoError(t, os.RemoveAll(walletDir)) - }() - - passwordFilePath := filepath.Join(testDir, passwordFileName) - require.NoError(t, ioutil.WriteFile(passwordFilePath, []byte(password), os.ModePerm)) - mnemonicFilePath := filepath.Join(testDir, mnemonicFileName) - require.NoError(t, ioutil.WriteFile(mnemonicFilePath, []byte(mnemonic), os.ModePerm)) - - numAccounts := int64(1) - app := cli.App{} - set := flag.NewFlagSet("test", 0) - set.String(flags.WalletDirFlag.Name, walletDir, "") - set.String(flags.WalletPasswordFileFlag.Name, passwordFilePath, "") - set.String(flags.KeymanagerKindFlag.Name, v2keymanager.Derived.String(), "") - set.String(flags.MnemonicFileFlag.Name, mnemonicFilePath, "") - set.Int64(flags.NumAccountsFlag.Name, numAccounts, "") - assert.NoError(t, set.Set(flags.WalletDirFlag.Name, walletDir)) - assert.NoError(t, set.Set(flags.WalletPasswordFileFlag.Name, passwordFilePath)) - assert.NoError(t, set.Set(flags.KeymanagerKindFlag.Name, v2keymanager.Derived.String())) - assert.NoError(t, set.Set(flags.MnemonicFileFlag.Name, mnemonicFilePath)) - assert.NoError(t, set.Set(flags.NumAccountsFlag.Name, strconv.Itoa(int(numAccounts)))) - cliCtx := cli.NewContext(&app, set, nil) - + cfg := setupRecoverCfg(t) + cfg.numAccounts = 1 + cliCtx := createRecoverCliCtx(t, cfg) require.NoError(t, RecoverWalletCli(cliCtx)) _, err := wallet.OpenWallet(cliCtx.Context, &wallet.Config{ - WalletDir: walletDir, + WalletDir: cfg.walletDir, WalletPassword: password, }) assert.NoError(t, err) } + +func TestRecoverDerivedWallet_AlreadyExists(t *testing.T) { + cfg := setupRecoverCfg(t) + cfg.numAccounts = 4 + cliCtx := createRecoverCliCtx(t, cfg) + require.NoError(t, RecoverWalletCli(cliCtx)) + + // Trying to recover an HD wallet into a directory that already exists should give an error + require.ErrorContains(t, "a wallet already exists at this location", RecoverWalletCli(cliCtx)) +} diff --git a/validator/rpc/auth.go b/validator/rpc/auth.go index 3dee7b877b..c05ec76afb 100644 --- a/validator/rpc/auth.go +++ b/validator/rpc/auth.go @@ -111,12 +111,24 @@ func (s *Server) createTokenString() (string, uint64, error) { // Initialize a wallet and send it over a global feed. func (s *Server) initializeWallet(ctx context.Context, cfg *wallet.Config) error { // We first ensure the user has a wallet. - if err := wallet.Exists(cfg.WalletDir); err != nil { - if errors.Is(err, wallet.ErrNoWalletFound) { - return wallet.ErrNoWalletFound - } - return errors.Wrap(err, "could not check if wallet exists") + exists, err := wallet.Exists(cfg.WalletDir) + if err != nil { + return errors.Wrap(err, wallet.CheckExistsErrMsg) } + if !exists { + return wallet.ErrNoWalletFound + } + valid, err := wallet.IsValid(cfg.WalletDir) + if err == wallet.ErrNoWalletFound { + return wallet.ErrNoWalletFound + } + if err != nil { + return errors.Wrap(err, wallet.CheckValidityErrMsg) + } + if !valid { + return errors.New(wallet.InvalidWalletErrMsg) + } + // We fire an event with the opened wallet over // a global feed signifying wallet initialization. w, err := wallet.OpenWallet(ctx, &wallet.Config{ diff --git a/validator/rpc/wallet.go b/validator/rpc/wallet.go index 6be929dc57..b93234d9af 100644 --- a/validator/rpc/wallet.go +++ b/validator/rpc/wallet.go @@ -7,7 +7,6 @@ import ( "path/filepath" ptypes "github.com/gogo/protobuf/types" - "github.com/pkg/errors" pb "github.com/prysmaticlabs/prysm/proto/validator/accounts/v2" "github.com/prysmaticlabs/prysm/shared/rand" v2 "github.com/prysmaticlabs/prysm/validator/accounts/v2" @@ -23,17 +22,36 @@ import ( var defaultWalletPath = filepath.Join(flags.DefaultValidatorDir(), flags.WalletDefaultDirName) +const ( + checkExistsErrMsg = "Could not check if wallet exists" + checkValidityErrMsg = "Could not check if wallet is valid" + noWalletMsg = "No wallet found at path" + invalidWalletMsg = "Directory does not contain a valid wallet" +) + // HasWallet checks if a user has created a wallet before as well as whether or not // they have used the web UI before to set a wallet password. func (s *Server) HasWallet(ctx context.Context, _ *ptypes.Empty) (*pb.HasWalletResponse, error) { - err := wallet.Exists(defaultWalletPath) - if err != nil && errors.Is(err, wallet.ErrNoWalletFound) { + exists, err := wallet.Exists(defaultWalletPath) + if err != nil { + return nil, status.Errorf(codes.Internal, checkExistsErrMsg) + } + if !exists { + return &pb.HasWalletResponse{ + WalletExists: false, + }, nil + } + valid, err := wallet.IsValid(defaultWalletPath) + if err == wallet.ErrNoWalletFound { return &pb.HasWalletResponse{ WalletExists: false, }, nil } if err != nil { - return nil, status.Errorf(codes.Internal, "Could not check if wallet exists: %v", err) + return nil, status.Errorf(codes.Internal, checkValidityErrMsg) + } + if !valid { + return nil, status.Errorf(codes.FailedPrecondition, invalidWalletMsg) } return &pb.HasWalletResponse{ WalletExists: true, @@ -43,6 +61,14 @@ func (s *Server) HasWallet(ctx context.Context, _ *ptypes.Empty) (*pb.HasWalletR // CreateWallet via an API request, allowing a user to save a new // derived, direct, or remote wallet. func (s *Server) CreateWallet(ctx context.Context, req *pb.CreateWalletRequest) (*pb.WalletResponse, error) { + // Currently defaultWalletPath is used as the wallet directory and req's WalletPath is ignored for simplicity + exists, err := wallet.Exists(defaultWalletPath) + if err != nil { + return nil, status.Errorf(codes.Internal, "Could not check for existing wallet: %v", err) + } + if exists { + return nil, status.Error(codes.AlreadyExists, "A wallet already exists at this location.") + } switch req.Keymanager { case pb.KeymanagerKind_DIRECT: // Needs to unmarshal the keystores from the requests. @@ -127,14 +153,25 @@ func (s *Server) EditConfig(ctx context.Context, req *pb.EditWalletConfigRequest // WalletConfig returns the wallet's configuration. If no wallet exists, we return an empty response. func (s *Server) WalletConfig(ctx context.Context, _ *ptypes.Empty) (*pb.WalletResponse, error) { - err := wallet.Exists(defaultWalletPath) - if err != nil && errors.Is(err, wallet.ErrNoWalletFound) { + exists, err := wallet.Exists(defaultWalletPath) + if err != nil { + return nil, status.Errorf(codes.Internal, checkExistsErrMsg) + } + if !exists { // If no wallet is found, we simply return an empty response. return &pb.WalletResponse{}, nil } - if err != nil { - return nil, status.Errorf(codes.Internal, "Could not check if wallet exists: %v", err) + valid, err := wallet.IsValid(defaultWalletPath) + if err == wallet.ErrNoWalletFound { + return &pb.WalletResponse{}, nil } + if err != nil { + return nil, status.Errorf(codes.Internal, checkValidityErrMsg) + } + if !valid { + return nil, status.Errorf(codes.FailedPrecondition, invalidWalletMsg) + } + if s.wallet == nil || s.keymanager == nil { // If no wallet is found, we simply return an empty response. return &pb.WalletResponse{}, nil @@ -189,12 +226,22 @@ func (s *Server) GenerateMnemonic(ctx context.Context, _ *ptypes.Empty) (*pb.Gen // ChangePassword allows changing a wallet password via the API as // an authenticated method. func (s *Server) ChangePassword(ctx context.Context, req *pb.ChangePasswordRequest) (*ptypes.Empty, error) { - err := wallet.Exists(defaultWalletPath) - if err != nil && errors.Is(err, wallet.ErrNoWalletFound) { - return nil, status.Error(codes.FailedPrecondition, "No wallet found") + exists, err := wallet.Exists(defaultWalletPath) + if err != nil { + return nil, status.Errorf(codes.Internal, checkExistsErrMsg) + } + if !exists { + return nil, status.Errorf(codes.FailedPrecondition, noWalletMsg) + } + valid, err := wallet.IsValid(defaultWalletPath) + if err == wallet.ErrNoWalletFound { + return nil, status.Errorf(codes.FailedPrecondition, noWalletMsg) } if err != nil { - return nil, status.Errorf(codes.Internal, "Could not check if wallet exists: %v", err) + return nil, status.Errorf(codes.Internal, checkValidityErrMsg) + } + if !valid { + return nil, status.Errorf(codes.FailedPrecondition, invalidWalletMsg) } if req.Password == "" { return nil, status.Error(codes.InvalidArgument, "Password cannot be empty") diff --git a/validator/rpc/wallet_test.go b/validator/rpc/wallet_test.go index e23ce6f2e3..7686cd761a 100644 --- a/validator/rpc/wallet_test.go +++ b/validator/rpc/wallet_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "os" "testing" ptypes "github.com/gogo/protobuf/types" @@ -34,6 +35,9 @@ func TestServer_CreateWallet_Direct(t *testing.T) { WalletPassword: strongPass, KeystoresPassword: strongPass, } + // We delete the directory at defaultWalletPath as CreateWallet will return an error if it tries to create a wallet + // where a directory already exists + require.NoError(t, os.RemoveAll(defaultWalletPath)) _, err := s.CreateWallet(ctx, req) require.ErrorContains(t, "No keystores included for import", err) @@ -80,6 +84,9 @@ func TestServer_CreateWallet_Derived(t *testing.T) { WalletPassword: strongPass, NumAccounts: 0, } + // We delete the directory at defaultWalletPath as CreateWallet will return an error if it tries to create a wallet + // where a directory already exists + require.NoError(t, os.RemoveAll(defaultWalletPath)) _, err := s.CreateWallet(ctx, req) require.ErrorContains(t, "Must create at least 1 validator account", err) @@ -93,6 +100,10 @@ func TestServer_CreateWallet_Derived(t *testing.T) { _, err = s.CreateWallet(ctx, req) require.NoError(t, err) + + // Now trying to create a wallet where a previous wallet already exists. We expect an error. + _, err = s.CreateWallet(ctx, req) + require.ErrorContains(t, "wallet already exists at this location", err) } func TestServer_WalletConfig_NoWalletFound(t *testing.T) { @@ -148,7 +159,7 @@ func TestServer_ChangePassword_Preconditions(t *testing.T) { _, err := ss.ChangePassword(ctx, &pb.ChangePasswordRequest{ Password: "", }) - assert.ErrorContains(t, "No wallet found", err) + assert.ErrorContains(t, noWalletMsg, err) // We attempt to create the wallet. w, err := v2.CreateWalletWithKeymanager(ctx, &v2.CreateWalletConfig{ WalletCfg: &wallet.Config{ @@ -242,11 +253,23 @@ func TestServer_HasWallet(t *testing.T) { ctx := context.Background() strongPass := "29384283xasjasd32%%&*@*#*" ss := &Server{} + // First delete the created folder and check the response + require.NoError(t, os.RemoveAll(defaultWalletPath)) resp, err := ss.HasWallet(ctx, &ptypes.Empty{}) require.NoError(t, err) assert.DeepEqual(t, &pb.HasWalletResponse{ WalletExists: false, }, resp) + + // We now create the folder but without a valid wallet, i.e. lacking a subdirectory such as 'direct' + // We expect an empty directory to behave similarly as if there were no directory + require.NoError(t, os.MkdirAll(defaultWalletPath, os.ModePerm)) + resp, err = ss.HasWallet(ctx, &ptypes.Empty{}) + require.NoError(t, err) + assert.DeepEqual(t, &pb.HasWalletResponse{ + WalletExists: false, + }, resp) + // We attempt to create the wallet. _, err = v2.CreateWalletWithKeymanager(ctx, &v2.CreateWalletConfig{ WalletCfg: &wallet.Config{