From e170c8ef36de9127b292d23cc42d80e6fe5b8632 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 13 Feb 2018 00:46:20 +0000 Subject: [PATCH 1/5] rpc: generate new account id for imported wallets --- ethcore/src/account_provider/mod.rs | 6 +++--- ethstore/src/ethstore.rs | 7 ++++++- ethstore/src/secret_store.rs | 2 +- rpc/src/v1/impls/parity_accounts.rs | 2 +- rpc/src/v1/tests/mocked/parity_accounts.rs | 22 +++++++++++++++++++++- 5 files changed, 32 insertions(+), 7 deletions(-) diff --git a/ethcore/src/account_provider/mod.rs b/ethcore/src/account_provider/mod.rs index d31ded682db..74d24e7f50e 100755 --- a/ethcore/src/account_provider/mod.rs +++ b/ethcore/src/account_provider/mod.rs @@ -264,9 +264,9 @@ impl AccountProvider { Ok(Address::from(account.address).into()) } - /// Import a new presale wallet. - pub fn import_wallet(&self, json: &[u8], password: &str) -> Result { - let account = self.sstore.import_wallet(SecretVaultRef::Root, json, password)?; + /// Import a new wallet. + pub fn import_wallet(&self, json: &[u8], password: &str, gen_id: bool) -> Result { + let account = self.sstore.import_wallet(SecretVaultRef::Root, json, password, gen_id)?; if self.blacklisted_accounts.contains(&account.address) { self.sstore.remove_account(&account, password)?; return Err(SSError::InvalidAccount.into()); diff --git a/ethstore/src/ethstore.rs b/ethstore/src/ethstore.rs index 4c6309a1102..40a687fc988 100755 --- a/ethstore/src/ethstore.rs +++ b/ethstore/src/ethstore.rs @@ -166,9 +166,14 @@ impl SecretStore for EthStore { self.insert_account(vault, keypair.secret().clone(), password) } - fn import_wallet(&self, vault: SecretVaultRef, json: &[u8], password: &str) -> Result { + fn import_wallet(&self, vault: SecretVaultRef, json: &[u8], password: &str, gen_id: bool) -> Result { let json_keyfile = json::KeyFile::load(json).map_err(|_| Error::InvalidKeyFile("Invalid JSON format".to_owned()))?; let mut safe_account = SafeAccount::from_file(json_keyfile, None); + + if gen_id { + safe_account.id = Random::random(); + } + let secret = safe_account.crypto.secret(password).map_err(|_| Error::InvalidPassword)?; safe_account.address = KeyPair::from_secret(secret)?.address(); self.store.import(vault, safe_account) diff --git a/ethstore/src/secret_store.rs b/ethstore/src/secret_store.rs index b46e6f2d817..ebac2f99229 100755 --- a/ethstore/src/secret_store.rs +++ b/ethstore/src/secret_store.rs @@ -116,7 +116,7 @@ pub trait SecretStore: SimpleSecretStore { /// Imports presale wallet fn import_presale(&self, vault: SecretVaultRef, json: &[u8], password: &str) -> Result; /// Imports existing JSON wallet - fn import_wallet(&self, vault: SecretVaultRef, json: &[u8], password: &str) -> Result; + fn import_wallet(&self, vault: SecretVaultRef, json: &[u8], password: &str, gen_id: bool) -> Result; /// Copies account between stores and vaults. fn copy_account(&self, new_store: &SimpleSecretStore, new_vault: SecretVaultRef, account: &StoreAccountRef, password: &str, new_password: &str) -> Result<(), Error>; /// Checks if password matches given account. diff --git a/rpc/src/v1/impls/parity_accounts.rs b/rpc/src/v1/impls/parity_accounts.rs index 128415bbd7f..8de8fea900f 100644 --- a/rpc/src/v1/impls/parity_accounts.rs +++ b/rpc/src/v1/impls/parity_accounts.rs @@ -95,7 +95,7 @@ impl ParityAccounts for ParityAccountsClient { let store = self.account_provider()?; store.import_presale(json.as_bytes(), &pass) - .or_else(|_| store.import_wallet(json.as_bytes(), &pass)) + .or_else(|_| store.import_wallet(json.as_bytes(), &pass, true)) .map(Into::into) .map_err(|e| errors::account("Could not create account.", e)) } diff --git a/rpc/src/v1/tests/mocked/parity_accounts.rs b/rpc/src/v1/tests/mocked/parity_accounts.rs index 540bea90394..258310b85a3 100644 --- a/rpc/src/v1/tests/mocked/parity_accounts.rs +++ b/rpc/src/v1/tests/mocked/parity_accounts.rs @@ -480,7 +480,7 @@ fn should_export_account() { // given let tester = setup(); let wallet = r#"{"id":"6a186c80-7797-cff2-bc2e-7c1d6a6cc76e","version":3,"crypto":{"cipher":"aes-128-ctr","cipherparams":{"iv":"a1c6ff99070f8032ca1c4e8add006373"},"ciphertext":"df27e3db64aa18d984b6439443f73660643c2d119a6f0fa2fa9a6456fc802d75","kdf":"pbkdf2","kdfparams":{"c":10240,"dklen":32,"prf":"hmac-sha256","salt":"ddc325335cda5567a1719313e73b4842511f3e4a837c9658eeb78e51ebe8c815"},"mac":"3dc888ae79cbb226ff9c455669f6cf2d79be72120f2298f6cb0d444fddc0aa3d"},"address":"0042e5d2a662eeaca8a7e828c174f98f35d8925b","name":"parity-export-test","meta":"{\"passwordHint\":\"parity-export-test\",\"timestamp\":1490017814987}"}"#; - tester.accounts.import_wallet(wallet.as_bytes(), "parity-export-test").unwrap(); + tester.accounts.import_wallet(wallet.as_bytes(), "parity-export-test", false).unwrap(); let accounts = tester.accounts.accounts().unwrap(); assert_eq!(accounts.len(), 1); @@ -501,6 +501,26 @@ fn should_export_account() { assert_eq!(result, Some(response.into())); } +#[test] +fn should_import_wallet() { + let tester = setup(); + + let id = "6a186c80-7797-cff2-bc2e-7c1d6a6cc76e"; + let request = r#"{"jsonrpc":"2.0","method":"parity_newAccountFromWallet","params":["{\"id\":\"\",\"version\":3,\"crypto\":{\"cipher\":\"aes-128-ctr\",\"cipherparams\":{\"iv\":\"478736fb55872c1baf01b27b1998c90b\"},\"ciphertext\":\"fe5a63cc0055d7b0b3b57886f930ad9b63f48950d1348145d95996c41e05f4e0\",\"kdf\":\"pbkdf2\",\"kdfparams\":{\"c\":10240,\"dklen\":32,\"prf\":\"hmac-sha256\",\"salt\":\"658436d6738a19731149a98744e5cf02c8d5aa1f8e80c1a43cc9351c70a984e4\"},\"mac\":\"c7384b26ecf25539d942030230062af9b69de5766cbcc4690bffce1536644631\"},\"address\":\"00bac56a8a27232baa044c03f43bf3648c961735\",\"name\":\"hello world\",\"meta\":\"{}\"}", "himom"],"id":1}"#; + let request = request.replace("", id); + let response = r#"{"jsonrpc":"2.0","result":"0x00bac56a8a27232baa044c03f43bf3648c961735","id":1}"#; + + let res = tester.io.handle_request_sync(&request).unwrap(); + + assert_eq!(res, response); + + let account_meta = tester.accounts.account_meta("0x00bac56a8a27232baa044c03f43bf3648c961735".into()).unwrap(); + let account_uuid: String = account_meta.uuid.unwrap().into(); + + // the RPC should import the account with a new id + assert!(account_uuid != id); +} + #[test] fn should_sign_message() { let tester = setup(); From 9346d291bb9cf27d2f0ec8a3ec357d7d3d19b6ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 13 Feb 2018 00:56:01 +0000 Subject: [PATCH 2/5] ethstore: handle duplicate wallet filenames --- ethstore/src/accounts_dir/disk.rs | 66 ++++++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/ethstore/src/accounts_dir/disk.rs b/ethstore/src/accounts_dir/disk.rs index 46a0ecf399b..db3a546ba50 100755 --- a/ethstore/src/accounts_dir/disk.rs +++ b/ethstore/src/accounts_dir/disk.rs @@ -153,8 +153,42 @@ impl DiskDirectory where T: KeyFileManager { ) } - /// insert account with given file name - pub fn insert_with_filename(&self, account: SafeAccount, filename: String) -> Result { + /// insert account with given file name. if the filename is a duplicate of any stored account, a number is appended + /// to the filename. + pub fn insert_with_filename(&self, account: SafeAccount, mut filename: String) -> Result { + // check for duplicate filenames and append/increment counter + let dups: Vec<_> = self.files()?.into_iter().filter_map(|f| { + match f.file_name().map(|n| n.to_string_lossy()) { + Some(ref name) if name.starts_with(&filename) => Some(name.to_string()), + _ => None + } + }).collect(); + + if !dups.is_empty() { + let mut max = 0; + let mut found = false; + + for dup in dups { + if dup.len() == filename.len() { + // we found a file with the same name + found = true; + } + else if dup[filename.len()..].starts_with("-") { + // this is a duplicate file with a counter appended + match dup[filename.len() + 1..].parse() { + Ok(n) if n > max => { + max = n; + }, + _ => {}, + } + } + } + + if found { + filename.push_str(&format!("-{}", max + 1)) + } + } + // update account filename let original_account = account.clone(); let mut account = account; @@ -317,6 +351,34 @@ mod test { let _ = fs::remove_dir_all(dir); } + #[test] + fn should_handle_duplicate_filenames() { + // given + let mut dir = env::temp_dir(); + dir.push("ethstore_should_handle_duplicate_filenames"); + let keypair = Random.generate().unwrap(); + let password = "hello world"; + let directory = RootDiskDirectory::create(dir.clone()).unwrap(); + + // when + let account = SafeAccount::create(&keypair, [0u8; 16], password, 1024, "Test".to_owned(), "{}".to_owned()); + let filename = "test".to_string(); + + directory.insert_with_filename(account.clone(), "foo".to_string()).unwrap(); + directory.insert_with_filename(account.clone(), "testa".to_string()).unwrap(); + let res1 = directory.insert_with_filename(account.clone(), filename.clone()); + let res2 = directory.insert_with_filename(account.clone(), filename.clone()); + let res3 = directory.insert_with_filename(account.clone(), filename.clone()); + + // then + assert_eq!(res1.unwrap().filename.unwrap(), filename); + assert_eq!(res2.unwrap().filename.unwrap(), format!("{}-1", filename)); + assert_eq!(res3.unwrap().filename.unwrap(), format!("{}-2", filename)); + + // cleanup + let _ = fs::remove_dir_all(dir); + } + #[test] fn should_manage_vaults() { // given From 4bf912dd56caba70818e05893ab0ad732ccc0be1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 13 Feb 2018 10:55:16 +0000 Subject: [PATCH 3/5] ethstore: simplify deduplication of wallet file names --- ethstore/src/accounts_dir/disk.rs | 70 +++++++++++-------------------- 1 file changed, 24 insertions(+), 46 deletions(-) diff --git a/ethstore/src/accounts_dir/disk.rs b/ethstore/src/accounts_dir/disk.rs index db3a546ba50..92878ef1eb5 100755 --- a/ethstore/src/accounts_dir/disk.rs +++ b/ethstore/src/accounts_dir/disk.rs @@ -153,56 +153,31 @@ impl DiskDirectory where T: KeyFileManager { ) } - /// insert account with given file name. if the filename is a duplicate of any stored account, a number is appended - /// to the filename. - pub fn insert_with_filename(&self, account: SafeAccount, mut filename: String) -> Result { - // check for duplicate filenames and append/increment counter - let dups: Vec<_> = self.files()?.into_iter().filter_map(|f| { - match f.file_name().map(|n| n.to_string_lossy()) { - Some(ref name) if name.starts_with(&filename) => Some(name.to_string()), - _ => None - } - }).collect(); - - if !dups.is_empty() { - let mut max = 0; - let mut found = false; - for dup in dups { - if dup.len() == filename.len() { - // we found a file with the same name - found = true; - } - else if dup[filename.len()..].starts_with("-") { - // this is a duplicate file with a counter appended - match dup[filename.len() + 1..].parse() { - Ok(n) if n > max => { - max = n; - }, - _ => {}, - } - } - } - - if found { - filename.push_str(&format!("-{}", max + 1)) - } + /// insert account with given filename. if the filename is a duplicate of any stored account, a random suffix is + /// appended to the filename. + pub fn insert_with_filename(&self, account: SafeAccount, mut filename: String) -> Result { + // path to keyfile + let mut keyfile_path = self.path.clone(); + keyfile_path.push(filename.as_str()); + + // check for duplicate filename and append random suffix + if keyfile_path.exists() { + let suffix = ::random::random_string(4); + filename.push_str(&format!("-{}", suffix)); + keyfile_path.set_file_name(&filename); } // update account filename let original_account = account.clone(); let mut account = account; - account.filename = Some(filename.clone()); + account.filename = Some(filename); { - // Path to keyfile - let mut keyfile_path = self.path.clone(); - keyfile_path.push(filename.as_str()); - // save the file let mut file = fs::File::create(&keyfile_path)?; - // Write key content + // write key content self.key_manager.write(original_account, &mut file).map_err(|e| Error::Custom(format!("{:?}", e)))?; file.flush()?; @@ -365,15 +340,18 @@ mod test { let filename = "test".to_string(); directory.insert_with_filename(account.clone(), "foo".to_string()).unwrap(); - directory.insert_with_filename(account.clone(), "testa".to_string()).unwrap(); - let res1 = directory.insert_with_filename(account.clone(), filename.clone()); - let res2 = directory.insert_with_filename(account.clone(), filename.clone()); - let res3 = directory.insert_with_filename(account.clone(), filename.clone()); + let file1 = directory.insert_with_filename(account.clone(), filename.clone()).unwrap().filename.unwrap(); + let file2 = directory.insert_with_filename(account.clone(), filename.clone()).unwrap().filename.unwrap(); + let file3 = directory.insert_with_filename(account.clone(), filename.clone()).unwrap().filename.unwrap(); // then - assert_eq!(res1.unwrap().filename.unwrap(), filename); - assert_eq!(res2.unwrap().filename.unwrap(), format!("{}-1", filename)); - assert_eq!(res3.unwrap().filename.unwrap(), format!("{}-2", filename)); + // the first file should have the original names + assert_eq!(file1, filename); + + // the following duplicate files should have a suffix appended + assert!(file2 != file3); + assert_eq!(file2.len(), filename.len() + 5); + assert_eq!(file3.len(), filename.len() + 5); // cleanup let _ = fs::remove_dir_all(dir); From 7ba0bc4da8a476a5fd50c0381092855ae9240e47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Tue, 13 Feb 2018 11:04:53 +0000 Subject: [PATCH 4/5] ethstore: do not dedup wallet filenames on update --- ethstore/src/accounts_dir/disk.rs | 37 +++++++++++++++++------------- ethstore/src/accounts_dir/vault.rs | 2 +- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/ethstore/src/accounts_dir/disk.rs b/ethstore/src/accounts_dir/disk.rs index 92878ef1eb5..dc817600a45 100755 --- a/ethstore/src/accounts_dir/disk.rs +++ b/ethstore/src/accounts_dir/disk.rs @@ -154,15 +154,15 @@ impl DiskDirectory where T: KeyFileManager { } - /// insert account with given filename. if the filename is a duplicate of any stored account, a random suffix is - /// appended to the filename. - pub fn insert_with_filename(&self, account: SafeAccount, mut filename: String) -> Result { + /// insert account with given filename. if the filename is a duplicate of any stored account and dedup is set to + /// true, a random suffix is appended to the filename. + pub fn insert_with_filename(&self, account: SafeAccount, mut filename: String, dedup: bool) -> Result { // path to keyfile let mut keyfile_path = self.path.clone(); keyfile_path.push(filename.as_str()); // check for duplicate filename and append random suffix - if keyfile_path.exists() { + if dedup && keyfile_path.exists() { let suffix = ::random::random_string(4); filename.push_str(&format!("-{}", suffix)); keyfile_path.set_file_name(&filename); @@ -209,17 +209,13 @@ impl KeyDirectory for DiskDirectory where T: KeyFileManager { fn update(&self, account: SafeAccount) -> Result { // Disk store handles updates correctly iff filename is the same - self.insert(account) + let filename = account_filename(&account); + self.insert_with_filename(account, filename, false) } fn insert(&self, account: SafeAccount) -> Result { - // build file path - let filename = account.filename.as_ref().cloned().unwrap_or_else(|| { - let timestamp = time::strftime("%Y-%m-%dT%H-%M-%S", &time::now_utc()).expect("Time-format string is valid."); - format!("UTC--{}Z--{}", timestamp, Uuid::from(account.id)) - }); - - self.insert_with_filename(account, filename) + let filename = account_filename(&account); + self.insert_with_filename(account, filename, true) } fn remove(&self, account: &SafeAccount) -> Result<(), Error> { @@ -295,6 +291,14 @@ impl KeyFileManager for DiskKeyFileManager { } } +fn account_filename(account: &SafeAccount) -> String { + // build file path + account.filename.as_ref().cloned().unwrap_or_else(|| { + let timestamp = time::strftime("%Y-%m-%dT%H-%M-%S", &time::now_utc()).expect("Time-format string is valid."); + format!("UTC--{}Z--{}", timestamp, Uuid::from(account.id)) + }) +} + #[cfg(test)] mod test { extern crate tempdir; @@ -338,11 +342,12 @@ mod test { // when let account = SafeAccount::create(&keypair, [0u8; 16], password, 1024, "Test".to_owned(), "{}".to_owned()); let filename = "test".to_string(); + let dedup = true; - directory.insert_with_filename(account.clone(), "foo".to_string()).unwrap(); - let file1 = directory.insert_with_filename(account.clone(), filename.clone()).unwrap().filename.unwrap(); - let file2 = directory.insert_with_filename(account.clone(), filename.clone()).unwrap().filename.unwrap(); - let file3 = directory.insert_with_filename(account.clone(), filename.clone()).unwrap().filename.unwrap(); + directory.insert_with_filename(account.clone(), "foo".to_string(), dedup).unwrap(); + let file1 = directory.insert_with_filename(account.clone(), filename.clone(), dedup).unwrap().filename.unwrap(); + let file2 = directory.insert_with_filename(account.clone(), filename.clone(), dedup).unwrap().filename.unwrap(); + let file3 = directory.insert_with_filename(account.clone(), filename.clone(), dedup).unwrap().filename.unwrap(); // then // the first file should have the original names diff --git a/ethstore/src/accounts_dir/vault.rs b/ethstore/src/accounts_dir/vault.rs index b270bb181d6..1ef85402a7a 100755 --- a/ethstore/src/accounts_dir/vault.rs +++ b/ethstore/src/accounts_dir/vault.rs @@ -106,7 +106,7 @@ impl VaultDiskDirectory { fn copy_to_vault(&self, vault: &VaultDiskDirectory) -> Result<(), Error> { for account in self.load()? { let filename = account.filename.clone().expect("self is instance of DiskDirectory; DiskDirectory fills filename in load; qed"); - vault.insert_with_filename(account, filename)?; + vault.insert_with_filename(account, filename, true)?; } Ok(()) From d7cc6a5d8c82ff0ff321db714a5537a6312f912e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Silva?= Date: Wed, 14 Feb 2018 12:00:31 +0000 Subject: [PATCH 5/5] ethstore: fix minor grumbles --- ethstore/src/accounts_dir/disk.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ethstore/src/accounts_dir/disk.rs b/ethstore/src/accounts_dir/disk.rs index dc817600a45..14461184406 100755 --- a/ethstore/src/accounts_dir/disk.rs +++ b/ethstore/src/accounts_dir/disk.rs @@ -158,8 +158,7 @@ impl DiskDirectory where T: KeyFileManager { /// true, a random suffix is appended to the filename. pub fn insert_with_filename(&self, account: SafeAccount, mut filename: String, dedup: bool) -> Result { // path to keyfile - let mut keyfile_path = self.path.clone(); - keyfile_path.push(filename.as_str()); + let mut keyfile_path = self.path.join(filename.as_str()); // check for duplicate filename and append random suffix if dedup && keyfile_path.exists() { @@ -293,7 +292,7 @@ impl KeyFileManager for DiskKeyFileManager { fn account_filename(account: &SafeAccount) -> String { // build file path - account.filename.as_ref().cloned().unwrap_or_else(|| { + account.filename.clone().unwrap_or_else(|| { let timestamp = time::strftime("%Y-%m-%dT%H-%M-%S", &time::now_utc()).expect("Time-format string is valid."); format!("UTC--{}Z--{}", timestamp, Uuid::from(account.id)) })