From 853aae2b9235b75e44b18e67c90bfc79797ef69e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 3 Feb 2017 13:56:48 +0100 Subject: [PATCH] Validate dapps accounts with address book (#4407) * Parametrize address book * Improving types in account_provider * Filtering dapps_accounts * Fixing RPC tests --- ethcore/src/account_provider/mod.rs | 50 ++++++++++++++++------ ethcore/src/account_provider/stores.rs | 49 ++++++++++----------- ethstore/src/ethstore.rs | 5 ++- ethstore/src/secret_store.rs | 3 +- rpc/src/v1/impls/parity.rs | 2 +- rpc/src/v1/impls/parity_accounts.rs | 38 ++++++++-------- rpc/src/v1/tests/mocked/eth.rs | 2 + rpc/src/v1/tests/mocked/parity_accounts.rs | 3 +- rpc/src/v1/traits/parity_accounts.rs | 2 +- 9 files changed, 89 insertions(+), 65 deletions(-) diff --git a/ethcore/src/account_provider/mod.rs b/ethcore/src/account_provider/mod.rs index c9e6a0fc6..46690fabd 100755 --- a/ethcore/src/account_provider/mod.rs +++ b/ethcore/src/account_provider/mod.rs @@ -21,7 +21,7 @@ mod stores; use self::stores::{AddressBook, DappsSettingsStore, NewDappsPolicy}; use std::fmt; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::time::{Instant, Duration}; use util::RwLock; use ethstore::{SimpleSecretStore, SecretStore, Error as SSError, EthStore, EthMultiStore, @@ -114,8 +114,8 @@ impl AccountProvider { pub fn new(sstore: Box) -> Self { AccountProvider { unlocked: RwLock::new(HashMap::new()), - address_book: RwLock::new(AddressBook::new(sstore.local_path().into())), - dapps_settings: RwLock::new(DappsSettingsStore::new(sstore.local_path().into())), + address_book: RwLock::new(AddressBook::new(&sstore.local_path())), + dapps_settings: RwLock::new(DappsSettingsStore::new(&sstore.local_path())), sstore: sstore, transient_sstore: transient_sstore(), } @@ -216,7 +216,7 @@ impl AccountProvider { Some(accounts) => Ok(accounts), None => match dapps.policy() { NewDappsPolicy::AllAccounts => self.accounts(), - NewDappsPolicy::Whitelist(accounts) => Ok(accounts), + NewDappsPolicy::Whitelist(accounts) => self.filter_addresses(accounts), } } } @@ -231,28 +231,42 @@ impl AccountProvider { /// Sets addresses visile for dapp. pub fn set_dapps_addresses(&self, dapp: DappId, addresses: Vec
) -> Result<(), Error> { + let addresses = self.filter_addresses(addresses)?; self.dapps_settings.write().set_accounts(dapp, addresses); Ok(()) } - /// Returns each address along with metadata. - pub fn addresses_info(&self) -> Result, Error> { - Ok(self.address_book.read().get()) + /// Removes addresses that are neither accounts nor in address book. + fn filter_addresses(&self, addresses: Vec
) -> Result, Error> { + let valid = self.addresses_info().into_iter() + .map(|(address, _)| address) + .chain(self.accounts()?) + .collect::>(); + + Ok(addresses.into_iter() + .filter(|a| valid.contains(&a)) + .collect() + ) } /// Returns each address along with metadata. - pub fn set_address_name(&self, account: Address, name: String) -> Result<(), Error> { - Ok(self.address_book.write().set_name(account, name)) + pub fn addresses_info(&self) -> HashMap { + self.address_book.read().get() } /// Returns each address along with metadata. - pub fn set_address_meta(&self, account: Address, meta: String) -> Result<(), Error> { - Ok(self.address_book.write().set_meta(account, meta)) + pub fn set_address_name(&self, account: Address, name: String) { + self.address_book.write().set_name(account, name) + } + + /// Returns each address along with metadata. + pub fn set_address_meta(&self, account: Address, meta: String) { + self.address_book.write().set_meta(account, meta) } /// Removes and address from the addressbook - pub fn remove_address(&self, addr: Address) -> Result<(), Error> { - Ok(self.address_book.write().remove(addr)) + pub fn remove_address(&self, addr: Address) { + self.address_book.write().remove(addr) } /// Returns each account along with name and meta. @@ -502,9 +516,12 @@ mod tests { let app = DappId("app1".into()); // set `AllAccounts` policy ap.set_new_dapps_whitelist(None).unwrap(); + // add accounts to address book + ap.set_address_name(1.into(), "1".into()); + ap.set_address_name(2.into(), "2".into()); // when - ap.set_dapps_addresses(app.clone(), vec![1.into(), 2.into()]).unwrap(); + ap.set_dapps_addresses(app.clone(), vec![1.into(), 2.into(), 3.into()]).unwrap(); // then assert_eq!(ap.dapps_addresses(app.clone()).unwrap(), vec![1.into(), 2.into()]); @@ -515,6 +532,7 @@ mod tests { // given let ap = AccountProvider::transient_provider(); let address = ap.new_account("test").unwrap(); + ap.set_address_name(1.into(), "1".into()); // When returning nothing ap.set_new_dapps_whitelist(Some(vec![])).unwrap(); @@ -524,6 +542,10 @@ mod tests { ap.set_new_dapps_whitelist(None).unwrap(); assert_eq!(ap.dapps_addresses("app1".into()).unwrap(), vec![address]); + // change to non-existent account + ap.set_new_dapps_whitelist(Some(vec![2.into()])).unwrap(); + assert_eq!(ap.dapps_addresses("app1".into()).unwrap(), vec![]); + // change to a whitelist ap.set_new_dapps_whitelist(Some(vec![1.into()])).unwrap(); assert_eq!(ap.dapps_addresses("app1".into()).unwrap(), vec![1.into()]); diff --git a/ethcore/src/account_provider/stores.rs b/ethcore/src/account_provider/stores.rs index d5d55e57b..e4bd7e1b9 100644 --- a/ethcore/src/account_provider/stores.rs +++ b/ethcore/src/account_provider/stores.rs @@ -19,7 +19,7 @@ use std::{fs, fmt, hash, ops}; use std::sync::atomic::{self, AtomicUsize}; use std::collections::HashMap; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use ethstore::ethkey::Address; use ethjson::misc::{ @@ -37,9 +37,9 @@ pub struct AddressBook { impl AddressBook { /// Creates new address book at given directory. - pub fn new(path: String) -> Self { + pub fn new(path: &Path) -> Self { let mut r = AddressBook { - cache: DiskMap::new(path, "address_book.json".into()) + cache: DiskMap::new(path, "address_book.json") }; r.cache.revert(AccountMeta::read); r @@ -200,11 +200,11 @@ pub struct DappsSettingsStore { impl DappsSettingsStore { /// Creates new store at given directory path. - pub fn new(path: String) -> Self { + pub fn new(path: &Path) -> Self { let mut r = DappsSettingsStore { - settings: DiskMap::new(path.clone(), "dapps_accounts.json".into()), - policy: DiskMap::new(path.clone(), "dapps_policy.json".into()), - history: DiskMap::new(path.clone(), "dapps_history.json".into()), + settings: DiskMap::new(path, "dapps_accounts.json".into()), + policy: DiskMap::new(path, "dapps_policy.json".into()), + history: DiskMap::new(path, "dapps_history.json".into()), time: TimeProvider::Clock, }; r.settings.revert(JsonSettings::read); @@ -297,9 +297,8 @@ impl ops::DerefMut for DiskMap { } impl DiskMap { - pub fn new(path: String, file_name: String) -> Self { - trace!(target: "diskmap", "new({})", path); - let mut path: PathBuf = path.into(); + pub fn new(path: &Path, file_name: &str) -> Self { + let mut path = path.to_owned(); path.push(file_name); trace!(target: "diskmap", "path={:?}", path); DiskMap { @@ -310,7 +309,7 @@ impl DiskMap { } pub fn transient() -> Self { - let mut map = DiskMap::new(Default::default(), "diskmap.json".into()); + let mut map = DiskMap::new(&PathBuf::new(), "diskmap.json".into()); map.transient = true; map } @@ -354,27 +353,25 @@ mod tests { #[test] fn should_save_and_reload_address_book() { - let temp = RandomTempPath::create_dir(); - let path = temp.as_str().to_owned(); - let mut b = AddressBook::new(path.clone()); + let path = RandomTempPath::create_dir(); + let mut b = AddressBook::new(&path); b.set_name(1.into(), "One".to_owned()); b.set_meta(1.into(), "{1:1}".to_owned()); - let b = AddressBook::new(path); + let b = AddressBook::new(&path); assert_eq!(b.get(), hash_map![1.into() => AccountMeta{name: "One".to_owned(), meta: "{1:1}".to_owned(), uuid: None}]); } #[test] fn should_remove_address() { - let temp = RandomTempPath::create_dir(); - let path = temp.as_str().to_owned(); - let mut b = AddressBook::new(path.clone()); + let path = RandomTempPath::create_dir(); + let mut b = AddressBook::new(&path); b.set_name(1.into(), "One".to_owned()); b.set_name(2.into(), "Two".to_owned()); b.set_name(3.into(), "Three".to_owned()); b.remove(2.into()); - let b = AddressBook::new(path); + let b = AddressBook::new(&path); assert_eq!(b.get(), hash_map![ 1.into() => AccountMeta{name: "One".to_owned(), meta: "{}".to_owned(), uuid: None}, 3.into() => AccountMeta{name: "Three".to_owned(), meta: "{}".to_owned(), uuid: None} @@ -384,15 +381,14 @@ mod tests { #[test] fn should_save_and_reload_dapps_settings() { // given - let temp = RandomTempPath::create_dir(); - let path = temp.as_str().to_owned(); - let mut b = DappsSettingsStore::new(path.clone()); + let path = RandomTempPath::create_dir(); + let mut b = DappsSettingsStore::new(&path); // when b.set_accounts("dappOne".into(), vec![1.into(), 2.into()]); // then - let b = DappsSettingsStore::new(path); + let b = DappsSettingsStore::new(&path); assert_eq!(b.settings(), hash_map![ "dappOne".into() => DappsSettings { accounts: vec![1.into(), 2.into()], @@ -422,9 +418,8 @@ mod tests { #[test] fn should_store_dapps_policy() { // given - let temp = RandomTempPath::create_dir(); - let path = temp.as_str().to_owned(); - let mut store = DappsSettingsStore::new(path.clone()); + let path = RandomTempPath::create_dir(); + let mut store = DappsSettingsStore::new(&path); // Test default policy assert_eq!(store.policy(), NewDappsPolicy::AllAccounts); @@ -433,7 +428,7 @@ mod tests { store.set_policy(NewDappsPolicy::Whitelist(vec![1.into(), 2.into()])); // then - let store = DappsSettingsStore::new(path); + let store = DappsSettingsStore::new(&path); assert_eq!(store.policy.clone(), hash_map![ "default".into() => NewDappsPolicy::Whitelist(vec![1.into(), 2.into()]) ]); diff --git a/ethstore/src/ethstore.rs b/ethstore/src/ethstore.rs index af521d56e..add5be129 100755 --- a/ethstore/src/ethstore.rs +++ b/ethstore/src/ethstore.rs @@ -16,6 +16,7 @@ use std::collections::{BTreeMap, HashMap}; use std::mem; +use std::path::PathBuf; use parking_lot::{Mutex, RwLock}; use crypto::KEY_ITERATIONS; @@ -164,8 +165,8 @@ impl SecretStore for EthStore { self.store.update(account_ref, old, safe_account) } - fn local_path(&self) -> String { - self.store.dir.path().map(|p| p.to_string_lossy().into_owned()).unwrap_or_else(|| String::new()) + fn local_path(&self) -> PathBuf { + self.store.dir.path().cloned().unwrap_or_else(PathBuf::new) } fn list_geth_accounts(&self, testnet: bool) -> Vec
{ diff --git a/ethstore/src/secret_store.rs b/ethstore/src/secret_store.rs index c90974f42..7042f434a 100755 --- a/ethstore/src/secret_store.rs +++ b/ethstore/src/secret_store.rs @@ -15,6 +15,7 @@ // along with Parity. If not, see . use std::hash::{Hash, Hasher}; +use std::path::PathBuf; use ethkey::{Address, Message, Signature, Secret, Public}; use Error; use json::Uuid; @@ -73,7 +74,7 @@ pub trait SecretStore: SimpleSecretStore { fn set_name(&self, account: &StoreAccountRef, name: String) -> Result<(), Error>; fn set_meta(&self, account: &StoreAccountRef, meta: String) -> Result<(), Error>; - fn local_path(&self) -> String; + fn local_path(&self) -> PathBuf; fn list_geth_accounts(&self, testnet: bool) -> Vec
; fn import_geth_accounts(&self, vault: SecretVaultRef, desired: Vec
, testnet: bool) -> Result, Error>; } diff --git a/rpc/src/v1/impls/parity.rs b/rpc/src/v1/impls/parity.rs index 185f094e7..055ccebeb 100644 --- a/rpc/src/v1/impls/parity.rs +++ b/rpc/src/v1/impls/parity.rs @@ -130,7 +130,7 @@ impl Parity for ParityClient where .into_iter().collect::>(); let info = store.accounts_info().map_err(|e| errors::account("Could not fetch account info.", e))?; - let other = store.addresses_info().expect("addresses_info always returns Ok; qed"); + let other = store.addresses_info(); Ok(info .into_iter() diff --git a/rpc/src/v1/impls/parity_accounts.rs b/rpc/src/v1/impls/parity_accounts.rs index 4348c2f42..05ac7e5e2 100644 --- a/rpc/src/v1/impls/parity_accounts.rs +++ b/rpc/src/v1/impls/parity_accounts.rs @@ -51,22 +51,27 @@ impl ParityAccountsClient where C: MiningBlockChainClient { } impl ParityAccounts for ParityAccountsClient where C: MiningBlockChainClient { - fn all_accounts_info(&self) -> Result>, Error> { + fn all_accounts_info(&self) -> Result>, Error> { self.active()?; let store = take_weak!(self.accounts); let info = store.accounts_info().map_err(|e| errors::account("Could not fetch account info.", e))?; - let other = store.addresses_info().expect("addresses_info always returns Ok; qed"); + let other = store.addresses_info(); - Ok(info.into_iter().chain(other.into_iter()).map(|(a, v)| { - let mut m = map![ - "name".to_owned() => v.name, - "meta".to_owned() => v.meta - ]; - if let &Some(ref uuid) = &v.uuid { - m.insert("uuid".to_owned(), format!("{}", uuid)); - } - (format!("0x{}", a.hex()), m) - }).collect()) + Ok(info + .into_iter() + .chain(other.into_iter()) + .map(|(address, v)| { + let mut m = map![ + "name".to_owned() => v.name, + "meta".to_owned() => v.meta + ]; + if let &Some(ref uuid) = &v.uuid { + m.insert("uuid".to_owned(), format!("{}", uuid)); + } + (address.into(), m) + }) + .collect() + ) } fn new_account_from_phrase(&self, phrase: String, pass: String) -> Result { @@ -132,8 +137,7 @@ impl ParityAccounts for ParityAccountsClient where C: MiningBlock let store = take_weak!(self.accounts); let addr: Address = addr.into(); - store.remove_address(addr) - .expect("remove_address always returns Ok; qed"); + store.remove_address(addr); Ok(true) } @@ -143,8 +147,7 @@ impl ParityAccounts for ParityAccountsClient where C: MiningBlock let addr: Address = addr.into(); store.set_account_name(addr.clone(), name.clone()) - .or_else(|_| store.set_address_name(addr, name)) - .expect("set_address_name always returns Ok; qed"); + .unwrap_or_else(|_| store.set_address_name(addr, name)); Ok(true) } @@ -154,8 +157,7 @@ impl ParityAccounts for ParityAccountsClient where C: MiningBlock let addr: Address = addr.into(); store.set_account_meta(addr.clone(), meta.clone()) - .or_else(|_| store.set_address_meta(addr, meta)) - .expect("set_address_meta always returns Ok; qed"); + .unwrap_or_else(|_| store.set_address_meta(addr, meta)); Ok(true) } diff --git a/rpc/src/v1/tests/mocked/eth.rs b/rpc/src/v1/tests/mocked/eth.rs index cc7beaf50..2b9239c7d 100644 --- a/rpc/src/v1/tests/mocked/eth.rs +++ b/rpc/src/v1/tests/mocked/eth.rs @@ -365,6 +365,8 @@ fn rpc_eth_accounts() { let tester = EthTester::default(); let address = tester.accounts_provider.new_account("").unwrap(); tester.accounts_provider.set_new_dapps_whitelist(None).unwrap(); + tester.accounts_provider.set_address_name(1.into(), "1".into()); + tester.accounts_provider.set_address_name(10.into(), "10".into()); // with current policy it should return the account let request = r#"{"jsonrpc": "2.0", "method": "eth_accounts", "params": [], "id": 1}"#; diff --git a/rpc/src/v1/tests/mocked/parity_accounts.rs b/rpc/src/v1/tests/mocked/parity_accounts.rs index 476e97229..be8cb3f91 100644 --- a/rpc/src/v1/tests/mocked/parity_accounts.rs +++ b/rpc/src/v1/tests/mocked/parity_accounts.rs @@ -120,10 +120,11 @@ fn should_be_able_to_set_meta() { fn rpc_parity_set_and_get_dapps_accounts() { // given let tester = setup(); + tester.accounts.set_address_name(10.into(), "10".into()); assert_eq!(tester.accounts.dapps_addresses("app1".into()).unwrap(), vec![]); // when - let request = r#"{"jsonrpc": "2.0", "method": "parity_setDappsAddresses","params":["app1",["0x000000000000000000000000000000000000000a"]], "id": 1}"#; + let request = r#"{"jsonrpc": "2.0", "method": "parity_setDappsAddresses","params":["app1",["0x000000000000000000000000000000000000000a","0x0000000000000000000000000000000000000001"]], "id": 1}"#; let response = r#"{"jsonrpc":"2.0","result":true,"id":1}"#; assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); diff --git a/rpc/src/v1/traits/parity_accounts.rs b/rpc/src/v1/traits/parity_accounts.rs index 1df707dc2..595a33740 100644 --- a/rpc/src/v1/traits/parity_accounts.rs +++ b/rpc/src/v1/traits/parity_accounts.rs @@ -25,7 +25,7 @@ build_rpc_trait! { pub trait ParityAccounts { /// Returns accounts information. #[rpc(name = "parity_allAccountsInfo")] - fn all_accounts_info(&self) -> Result>, Error>; + fn all_accounts_info(&self) -> Result>, Error>; /// Creates new account from the given phrase using standard brainwallet mechanism. /// Second parameter is password for the new account.