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

@@ -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<SecretStore>) -> Self {
 		AccountProvider {
 			unlocked: RwLock::new(HashMap::new()),
-			address_book: RwLock::new(AddressBook::new(sstore.local_path().into())),
+			address_book: RwLock::new(AddressBook::new(&sstore.local_path())),
-			dapps_settings: RwLock::new(DappsSettingsStore::new(sstore.local_path().into())),
+			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<Address>) -> Result<(), Error> {
+		let addresses = self.filter_addresses(addresses)?;
 		self.dapps_settings.write().set_accounts(dapp, addresses);
 		Ok(())
 	}
 
-	/// Returns each address along with metadata.
+	/// Removes addresses that are neither accounts nor in address book.
-	pub fn addresses_info(&self) -> Result<HashMap<Address, AccountMeta>, Error> {
+	fn filter_addresses(&self, addresses: Vec<Address>) -> Result<Vec<Address>, Error> {
-		Ok(self.address_book.read().get())
+		let valid = self.addresses_info().into_iter()
+			.map(|(address, _)| address)
+			.chain(self.accounts()?)
+			.collect::<HashSet<_>>();
+
+		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> {
+	pub fn addresses_info(&self) -> HashMap<Address, AccountMeta> {
-		Ok(self.address_book.write().set_name(account, name))
+		self.address_book.read().get()
 	}
 
 	/// Returns each address along with metadata.
-	pub fn set_address_meta(&self, account: Address, meta: String) -> Result<(), Error> {
+	pub fn set_address_name(&self, account: Address, name: String) {
-		Ok(self.address_book.write().set_meta(account, meta))
+		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> {
+	pub fn remove_address(&self, addr: Address) {
-		Ok(self.address_book.write().remove(addr))
+		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()]);

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()),
+			settings: DiskMap::new(path, "dapps_accounts.json".into()),
-			policy: DiskMap::new(path.clone(), "dapps_policy.json".into()),
+			policy: DiskMap::new(path, "dapps_policy.json".into()),
-			history: DiskMap::new(path.clone(), "dapps_history.json".into()),
+			history: DiskMap::new(path, "dapps_history.json".into()),
 			time: TimeProvider::Clock,
 		};
 		r.settings.revert(JsonSettings::read);
@@ -297,9 +297,8 @@ impl<K: hash::Hash + Eq, V> ops::DerefMut for DiskMap<K, V> {
 }
 
 impl<K: hash::Hash + Eq, V> DiskMap<K, V> {
-	pub fn new(path: String, file_name: String) -> Self {
+	pub fn new(path: &Path, file_name: &str) -> Self {
-		trace!(target: "diskmap", "new({})", path);
+		let mut path = path.to_owned();
-		let mut path: PathBuf = path.into();
 		path.push(file_name);
 		trace!(target: "diskmap", "path={:?}", path);
 		DiskMap {
@@ -310,7 +309,7 @@ impl<K: hash::Hash + Eq, V> DiskMap<K, V> {
 	}
 
 	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 = RandomTempPath::create_dir();
-		let path = temp.as_str().to_owned();
+		let mut b = AddressBook::new(&path);
-		let mut b = AddressBook::new(path.clone());
 		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 = RandomTempPath::create_dir();
-		let path = temp.as_str().to_owned();
+		let mut b = AddressBook::new(&path);
-		let mut b = AddressBook::new(path.clone());
 
 		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 = RandomTempPath::create_dir();
-		let path = temp.as_str().to_owned();
+		let mut b = DappsSettingsStore::new(&path);
-		let mut b = DappsSettingsStore::new(path.clone());
 
 		// 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 = RandomTempPath::create_dir();
-		let path = temp.as_str().to_owned();
+		let mut store = DappsSettingsStore::new(&path);
-		let mut store = DappsSettingsStore::new(path.clone());
 
 		// 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()])
 		]);

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 {
+	fn local_path(&self) -> PathBuf {
-		self.store.dir.path().map(|p| p.to_string_lossy().into_owned()).unwrap_or_else(|| String::new())
+		self.store.dir.path().cloned().unwrap_or_else(PathBuf::new)
 	}
 
 	fn list_geth_accounts(&self, testnet: bool) -> Vec<Address> {

ethstore/src/secret_store.rs

@@ -15,6 +15,7 @@
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
 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<Address>;
 	fn import_geth_accounts(&self, vault: SecretVaultRef, desired: Vec<Address>, testnet: bool) -> Result<Vec<StoreAccountRef>, Error>;
 }

rpc/src/v1/impls/parity.rs

@@ -130,7 +130,7 @@ impl<C, M, S: ?Sized, U> Parity for ParityClient<C, M, S, U> where
 			.into_iter().collect::<HashSet<_>>();
 
 		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()

rpc/src/v1/impls/parity_accounts.rs

@@ -51,13 +51,16 @@ impl<C> ParityAccountsClient<C> where C: MiningBlockChainClient {
 }
 
 impl<C: 'static> ParityAccounts for ParityAccountsClient<C> where C: MiningBlockChainClient {
-	fn all_accounts_info(&self) -> Result<BTreeMap<String, BTreeMap<String, String>>, Error> {
+	fn all_accounts_info(&self) -> Result<BTreeMap<RpcH160, BTreeMap<String, String>>, 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)| {
+		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
@@ -65,8 +68,10 @@ impl<C: 'static> ParityAccounts for ParityAccountsClient<C> where C: MiningBlock
 			   if let &Some(ref uuid) = &v.uuid {
 				   m.insert("uuid".to_owned(), format!("{}", uuid));
 			   }
-			(format!("0x{}", a.hex()), m)
+			   (address.into(), m)
-		}).collect())
+		   })
+		   .collect()
+		)
 	}
 
 	fn new_account_from_phrase(&self, phrase: String, pass: String) -> Result<RpcH160, Error> {
@@ -132,8 +137,7 @@ impl<C: 'static> ParityAccounts for ParityAccountsClient<C> where C: MiningBlock
 		let store = take_weak!(self.accounts);
 		let addr: Address = addr.into();
 
-		store.remove_address(addr)
+		store.remove_address(addr);
-			.expect("remove_address always returns Ok; qed");
 		Ok(true)
 	}
 
@@ -143,8 +147,7 @@ impl<C: 'static> ParityAccounts for ParityAccountsClient<C> where C: MiningBlock
 		let addr: Address = addr.into();
 
 		store.set_account_name(addr.clone(), name.clone())
-			.or_else(|_| store.set_address_name(addr, name))
+			.unwrap_or_else(|_| store.set_address_name(addr, name));
-			.expect("set_address_name always returns Ok; qed");
 		Ok(true)
 	}
 
@@ -154,8 +157,7 @@ impl<C: 'static> ParityAccounts for ParityAccountsClient<C> where C: MiningBlock
 		let addr: Address = addr.into();
 
 		store.set_account_meta(addr.clone(), meta.clone())
-			.or_else(|_| store.set_address_meta(addr, meta))
+			.unwrap_or_else(|_| store.set_address_meta(addr, meta));
-			.expect("set_address_meta always returns Ok; qed");
 		Ok(true)
 	}
 

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}"#;

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()));
 

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<BTreeMap<String, BTreeMap<String, String>>, Error>;
+		fn all_accounts_info(&self) -> Result<BTreeMap<H160, BTreeMap<String, String>>, Error>;
 
 		/// Creates new account from the given phrase using standard brainwallet mechanism.
 		/// Second parameter is password for the new account.