From 4696d7f60656a83ee5f479f10e586688f19c63d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Sat, 10 Dec 2016 12:34:20 +0100 Subject: [PATCH] Additional RPCs for dapps accounts management --- ethcore/src/account_provider/mod.rs | 58 +++++++- ethcore/src/account_provider/stores.rs | 161 +++++++++++++++++---- json/src/misc/dapps_settings.rs | 29 ++++ json/src/misc/mod.rs | 2 +- rpc/src/v1/impls/eth.rs | 6 +- rpc/src/v1/impls/parity_accounts.rs | 52 +++++-- rpc/src/v1/tests/mocked/eth.rs | 10 +- rpc/src/v1/tests/mocked/parity_accounts.rs | 48 +++++- rpc/src/v1/traits/parity_accounts.rs | 18 +++ rpc/src/v1/types/dapp_id.rs | 6 + 10 files changed, 341 insertions(+), 49 deletions(-) diff --git a/ethcore/src/account_provider/mod.rs b/ethcore/src/account_provider/mod.rs index a2c83f1ce..72a546b1f 100644 --- a/ethcore/src/account_provider/mod.rs +++ b/ethcore/src/account_provider/mod.rs @@ -18,7 +18,7 @@ mod stores; -use self::stores::{AddressBook, DappsSettingsStore}; +use self::stores::{AddressBook, DappsSettingsStore, NewDappsPolicy}; use std::fmt; use std::collections::HashMap; @@ -167,10 +167,49 @@ impl AccountProvider { Ok(accounts) } + /// Sets a whitelist of accounts exposed for unknown dapps. + /// `None` means that all accounts will be visible. + pub fn set_new_dapps_whitelist(&self, accounts: Option>) -> Result<(), Error> { + self.dapps_settings.write().set_policy(match accounts { + None => NewDappsPolicy::AllAccounts, + Some(accounts) => NewDappsPolicy::Whitelist(accounts), + }); + Ok(()) + } + + /// Gets a whitelist of accounts exposed for unknown dapps. + /// `None` means that all accounts will be visible. + pub fn new_dapps_whitelist(&self) -> Result>, Error> { + Ok(match self.dapps_settings.read().policy() { + NewDappsPolicy::AllAccounts => None, + NewDappsPolicy::Whitelist(accounts) => Some(accounts), + }) + } + + /// Gets a list of dapps recently requesting accounts. + pub fn recent_dapps(&self) -> Result, Error> { + Ok(self.dapps_settings.read().recent_dapps()) + } + + /// Marks dapp as recently used. + pub fn note_dapp_used(&self, dapp: DappId) -> Result<(), Error> { + let mut dapps = self.dapps_settings.write(); + dapps.mark_dapp_used(dapp.clone()); + Ok(()) + } + /// Gets addresses visile for dapp. pub fn dapps_addresses(&self, dapp: DappId) -> Result, Error> { - let accounts = self.dapps_settings.read().get(); - Ok(accounts.get(&dapp).map(|settings| settings.accounts.clone()).unwrap_or_else(Vec::new)) + let dapps = self.dapps_settings.read(); + + let accounts = dapps.settings().get(&dapp).map(|settings| settings.accounts.clone()); + match accounts { + Some(accounts) => Ok(accounts), + None => match dapps.policy() { + NewDappsPolicy::AllAccounts => self.accounts(), + NewDappsPolicy::Whitelist(accounts) => Ok(accounts), + } + } } /// Sets addresses visile for dapp. @@ -386,4 +425,17 @@ mod tests { // then assert_eq!(ap.dapps_addresses(app.clone()).unwrap(), vec![1.into(), 2.into()]); } + + #[test] + fn should_set_dapps_policy() { + // given + let ap = AccountProvider::transient_provider(); + let address = ap.new_account("test").unwrap(); + // Default policy should be to return all + assert_eq!(ap.dapps_addresses("app1".into()).unwrap(), vec![address]); + + // change policy + 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 8bf555d68..69cf29468 100644 --- a/ethcore/src/account_provider/stores.rs +++ b/ethcore/src/account_provider/stores.rs @@ -17,11 +17,11 @@ //! Address Book and Dapps Settings Store use std::{fs, fmt, hash, ops}; -use std::collections::HashMap; +use std::collections::{HashMap, VecDeque}; use std::path::PathBuf; use ethstore::ethkey::Address; -use ethjson::misc::{AccountMeta, DappsSettings as JsonSettings}; +use ethjson::misc::{AccountMeta, DappsSettings as JsonSettings, NewDappsPolicy as JsonNewDappsPolicy}; use account_provider::DappId; /// Disk-backed map from Address to String. Uses JSON. @@ -105,43 +105,106 @@ impl From for JsonSettings { } } +/// Dapps user settings +#[derive(Debug, Clone, Eq, PartialEq)] +pub enum NewDappsPolicy { + AllAccounts, + Whitelist(Vec
), +} + +impl From for NewDappsPolicy { + fn from(s: JsonNewDappsPolicy) -> Self { + match s { + JsonNewDappsPolicy::AllAccounts => NewDappsPolicy::AllAccounts, + JsonNewDappsPolicy::Whitelist(accounts) => NewDappsPolicy::Whitelist( + accounts.into_iter().map(Into::into).collect() + ), + } + } +} + +impl From for JsonNewDappsPolicy { + fn from(s: NewDappsPolicy) -> Self { + match s { + NewDappsPolicy::AllAccounts => JsonNewDappsPolicy::AllAccounts, + NewDappsPolicy::Whitelist(accounts) => JsonNewDappsPolicy::Whitelist( + accounts.into_iter().map(Into::into).collect() + ), + } + } +} + +const MAX_RECENT_DAPPS: usize = 10; + /// Disk-backed map from DappId to Settings. Uses JSON. pub struct DappsSettingsStore { - cache: DiskMap, + /// Dapps Settings + settings: DiskMap, + /// New Dapps Policy + policy: DiskMap, + /// Recently Accessed Dapps (transient) + recent: VecDeque, } impl DappsSettingsStore { /// Creates new store at given directory path. pub fn new(path: String) -> Self { let mut r = DappsSettingsStore { - cache: DiskMap::new(path, "dapps_accounts.json".into()) + settings: DiskMap::new(path.clone(), "dapps_accounts.json".into()), + policy: DiskMap::new(path.clone(), "dapps_policy.json".into()), + recent: VecDeque::with_capacity(MAX_RECENT_DAPPS), }; - r.cache.revert(JsonSettings::read_dapps_settings); + r.settings.revert(JsonSettings::read_dapps_settings); + r.policy.revert(JsonNewDappsPolicy::read_new_dapps_policy); r } /// Creates transient store (no changes are saved to disk). pub fn transient() -> Self { DappsSettingsStore { - cache: DiskMap::transient() + settings: DiskMap::transient(), + policy: DiskMap::transient(), + recent: VecDeque::with_capacity(MAX_RECENT_DAPPS), } } /// Get copy of the dapps settings - pub fn get(&self) -> HashMap { - self.cache.clone() + pub fn settings(&self) -> HashMap { + self.settings.clone() } - fn save(&self) { - self.cache.save(JsonSettings::write_dapps_settings) + /// Returns current new dapps policy + pub fn policy(&self) -> NewDappsPolicy { + self.policy.get("default").cloned().unwrap_or(NewDappsPolicy::AllAccounts) } + /// Returns recent dapps (in order of last request) + pub fn recent_dapps(&self) -> Vec { + self.recent.iter().cloned().collect() + } + + /// Marks recent dapp as used + pub fn mark_dapp_used(&mut self, dapp: DappId) { + self.recent.retain(|id| id != &dapp); + self.recent.push_front(dapp); + while self.recent.len() > MAX_RECENT_DAPPS { + self.recent.pop_back(); + } + } + + /// Sets current new dapps policy + pub fn set_policy(&mut self, policy: NewDappsPolicy) { + self.policy.insert("default".into(), policy); + self.policy.save(JsonNewDappsPolicy::write_new_dapps_policy); + } + + /// Sets accounts for specific dapp. pub fn set_accounts(&mut self, id: DappId, accounts: Vec
) { { - let mut settings = self.cache.entry(id).or_insert_with(DappsSettings::default); + let mut settings = self.settings.entry(id).or_insert_with(DappsSettings::default); settings.accounts = accounts; } - self.save(); + self.settings.save(JsonSettings::write_dapps_settings); } } @@ -216,7 +279,7 @@ impl DiskMap { #[cfg(test)] mod tests { - use super::{AddressBook, DappsSettingsStore, DappsSettings}; + use super::{AddressBook, DappsSettingsStore, DappsSettings, NewDappsPolicy}; use std::collections::HashMap; use ethjson::misc::AccountMeta; use devtools::RandomTempPath; @@ -232,25 +295,6 @@ mod tests { assert_eq!(b.get(), hash_map![1.into() => AccountMeta{name: "One".to_owned(), meta: "{1:1}".to_owned(), uuid: None}]); } - #[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()); - - // when - b.set_accounts("dappOne".into(), vec![1.into(), 2.into()]); - - // then - let b = DappsSettingsStore::new(path); - assert_eq!(b.get(), hash_map![ - "dappOne".into() => DappsSettings { - accounts: vec![1.into(), 2.into()], - } - ]); - } - #[test] fn should_remove_address() { let temp = RandomTempPath::create_dir(); @@ -268,4 +312,57 @@ mod tests { 3.into() => AccountMeta{name: "Three".to_owned(), meta: "{}".to_owned(), uuid: None} ]); } + + #[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()); + + // when + b.set_accounts("dappOne".into(), vec![1.into(), 2.into()]); + + // then + let b = DappsSettingsStore::new(path); + assert_eq!(b.settings(), hash_map![ + "dappOne".into() => DappsSettings { + accounts: vec![1.into(), 2.into()], + } + ]); + } + + #[test] + fn should_maintain_a_list_of_recent_dapps() { + let mut store = DappsSettingsStore::transient(); + assert!(store.recent_dapps().is_empty(), "Initially recent dapps should be empty."); + + store.mark_dapp_used("dapp1".into()); + assert_eq!(store.recent_dapps(), vec!["dapp1".to_owned()]); + + store.mark_dapp_used("dapp2".into()); + assert_eq!(store.recent_dapps(), vec!["dapp2".to_owned(), "dapp1".to_owned()]); + + store.mark_dapp_used("dapp1".into()); + assert_eq!(store.recent_dapps(), vec!["dapp1".to_owned(), "dapp2".to_owned()]); + } + + #[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()); + // Test default policy + assert_eq!(store.policy(), NewDappsPolicy::AllAccounts); + + // when + store.set_policy(NewDappsPolicy::Whitelist(vec![1.into(), 2.into()])); + + // then + let store = DappsSettingsStore::new(path); + assert_eq!(store.policy.clone(), hash_map![ + "default".into() => NewDappsPolicy::Whitelist(vec![1.into(), 2.into()]) + ]); + } } diff --git a/json/src/misc/dapps_settings.rs b/json/src/misc/dapps_settings.rs index 893e7e93e..dc140fd29 100644 --- a/json/src/misc/dapps_settings.rs +++ b/json/src/misc/dapps_settings.rs @@ -49,3 +49,32 @@ impl DappsSettings { serde_json::to_writer(writer, &m.iter().map(|(a, m)| (a.clone().into(), m.clone().into())).collect::>()) } } + +/// Accounts policy for new dapps. +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] +pub enum NewDappsPolicy { + /// All accounts are exposed by default. + AllAccounts, + /// Only accounts listed here are exposed by default for new dapps. + Whitelist(Vec), +} + +impl NewDappsPolicy { + /// Read a hash map of `String -> NewDappsPolicy` + pub fn read_new_dapps_policy(reader: R) -> Result, serde_json::Error> where + R: io::Read, + S: From + Clone, + { + serde_json::from_reader(reader).map(|ok: HashMap| + ok.into_iter().map(|(a, m)| (a.into(), m.into())).collect() + ) + } + + /// Write a hash map of `String -> NewDappsPolicy` + pub fn write_new_dapps_policy(m: &HashMap, writer: &mut W) -> Result<(), serde_json::Error> where + W: io::Write, + S: Into + Clone, + { + serde_json::to_writer(writer, &m.iter().map(|(a, m)| (a.clone().into(), m.clone().into())).collect::>()) + } +} diff --git a/json/src/misc/mod.rs b/json/src/misc/mod.rs index baab83d08..0fe89e850 100644 --- a/json/src/misc/mod.rs +++ b/json/src/misc/mod.rs @@ -19,5 +19,5 @@ mod account_meta; mod dapps_settings; -pub use self::dapps_settings::DappsSettings; +pub use self::dapps_settings::{DappsSettings, NewDappsPolicy}; pub use self::account_meta::AccountMeta; diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index 6b9f47de3..44e7d9ead 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -340,7 +340,11 @@ impl Eth for EthClient where let dapp = id.0; let store = take_weak!(self.accounts); - let accounts = try!(store.dapps_addresses(dapp.into()).map_err(|e| errors::internal("Could not fetch accounts.", e))); + let accounts = try!(store + .note_dapp_used(dapp.clone().into()) + .and_then(|_| store.dapps_addresses(dapp.into())) + .map_err(|e| errors::internal("Could not fetch accounts.", e)) + ); Ok(accounts.into_iter().map(Into::into).collect()) } diff --git a/rpc/src/v1/impls/parity_accounts.rs b/rpc/src/v1/impls/parity_accounts.rs index 188e2290e..b420548f2 100644 --- a/rpc/src/v1/impls/parity_accounts.rs +++ b/rpc/src/v1/impls/parity_accounts.rs @@ -164,19 +164,51 @@ impl ParityAccounts for ParityAccountsClient where C: MiningBlock fn set_dapps_addresses(&self, dapp: DappId, addresses: Vec) -> Result { let store = take_weak!(self.accounts); - let addresses = addresses.into_iter().map(Into::into).collect(); - store.set_dapps_addresses(dapp.into(), addresses) + store.set_dapps_addresses(dapp.into(), into_vec(addresses)) .map_err(|e| errors::account("Couldn't set dapps addresses.", e)) .map(|_| true) } + fn dapps_addresses(&self, dapp: DappId) -> Result, Error> { + let store = take_weak!(self.accounts); + + store.dapps_addresses(dapp.into()) + .map_err(|e| errors::account("Couldn't get dapps addresses.", e)) + .map(into_vec) + } + + fn set_new_dapps_whitelist(&self, whitelist: Option>) -> Result { + let store = take_weak!(self.accounts); + + store + .set_new_dapps_whitelist(whitelist.map(into_vec)) + .map_err(|e| errors::account("Couldn't set dapps whitelist.", e)) + .map(|_| true) + } + + fn new_dapps_whitelist(&self) -> Result>, Error> { + let store = take_weak!(self.accounts); + + store.new_dapps_whitelist() + .map_err(|e| errors::account("Couldn't get dapps whitelist.", e)) + .map(|accounts| accounts.map(into_vec)) + } + + fn recent_dapps(&self) -> Result, Error> { + let store = take_weak!(self.accounts); + + store.recent_dapps() + .map_err(|e| errors::account("Couldn't get recent dapps.", e)) + .map(into_vec) + } + fn import_geth_accounts(&self, addresses: Vec) -> Result, Error> { let store = take_weak!(self.accounts); store - .import_geth_accounts(addresses.into_iter().map(Into::into).collect(), false) - .map(|imported| imported.into_iter().map(Into::into).collect()) + .import_geth_accounts(into_vec(addresses), false) + .map(into_vec) .map_err(|e| errors::account("Couldn't import Geth accounts", e)) } @@ -184,10 +216,12 @@ impl ParityAccounts for ParityAccountsClient where C: MiningBlock try!(self.active()); let store = take_weak!(self.accounts); - Ok(store.list_geth_accounts(false) - .into_iter() - .map(Into::into) - .collect() - ) + Ok(into_vec(store.list_geth_accounts(false))) } } + +fn into_vec(a: Vec) -> Vec where + A: Into +{ + a.into_iter().map(Into::into).collect() +} diff --git a/rpc/src/v1/tests/mocked/eth.rs b/rpc/src/v1/tests/mocked/eth.rs index 05c95346d..aa60aeb2e 100644 --- a/rpc/src/v1/tests/mocked/eth.rs +++ b/rpc/src/v1/tests/mocked/eth.rs @@ -354,11 +354,17 @@ fn rpc_eth_gas_price() { #[test] fn rpc_eth_accounts() { let tester = EthTester::default(); - let _address = tester.accounts_provider.new_account("").unwrap(); + let address = tester.accounts_provider.new_account("").unwrap(); + // with default policy it should return the account + let request = r#"{"jsonrpc": "2.0", "method": "eth_accounts", "params": [], "id": 1}"#; + let response = r#"{"jsonrpc":"2.0","result":[""#.to_owned() + &format!("0x{:?}", address) + r#""],"id":1}"#; + assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); + + tester.accounts_provider.set_new_dapps_whitelist(Some(vec![1.into()])).unwrap(); // even with some account it should return empty list (no dapp detected) let request = r#"{"jsonrpc": "2.0", "method": "eth_accounts", "params": [], "id": 1}"#; - let response = r#"{"jsonrpc":"2.0","result":[],"id":1}"#; + let response = r#"{"jsonrpc":"2.0","result":["0x0000000000000000000000000000000000000001"],"id":1}"#; assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); // when we add visible address it should return that. diff --git a/rpc/src/v1/tests/mocked/parity_accounts.rs b/rpc/src/v1/tests/mocked/parity_accounts.rs index 8b42500e6..66597ca6f 100644 --- a/rpc/src/v1/tests/mocked/parity_accounts.rs +++ b/rpc/src/v1/tests/mocked/parity_accounts.rs @@ -117,7 +117,7 @@ fn should_be_able_to_set_meta() { } #[test] -fn rpc_parity_set_dapps_accounts() { +fn rpc_parity_set_and_get_dapps_accounts() { // given let tester = setup(); assert_eq!(tester.accounts.dapps_addresses("app1".into()).unwrap(), vec![]); @@ -129,6 +129,52 @@ fn rpc_parity_set_dapps_accounts() { // then assert_eq!(tester.accounts.dapps_addresses("app1".into()).unwrap(), vec![10.into()]); + let request = r#"{"jsonrpc": "2.0", "method": "parity_getDappsAddresses","params":["app1"], "id": 1}"#; + let response = r#"{"jsonrpc":"2.0","result":["0x000000000000000000000000000000000000000a"],"id":1}"#; + assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); +} + +#[test] +fn rpc_parity_set_and_get_new_dapps_whitelist() { + // given + let tester = setup(); + + // when set to whitelist + let request = r#"{"jsonrpc": "2.0", "method": "parity_setNewDappsWhitelist","params":[["0x000000000000000000000000000000000000000a"]], "id": 1}"#; + let response = r#"{"jsonrpc":"2.0","result":true,"id":1}"#; + assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); + + // then + assert_eq!(tester.accounts.new_dapps_whitelist().unwrap(), Some(vec![10.into()])); + let request = r#"{"jsonrpc": "2.0", "method": "parity_getNewDappsWhitelist","params":[], "id": 1}"#; + let response = r#"{"jsonrpc":"2.0","result":["0x000000000000000000000000000000000000000a"],"id":1}"#; + assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); + + // when set to empty + let request = r#"{"jsonrpc": "2.0", "method": "parity_setNewDappsWhitelist","params":[null], "id": 1}"#; + let response = r#"{"jsonrpc":"2.0","result":true,"id":1}"#; + assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); + + // then + assert_eq!(tester.accounts.new_dapps_whitelist().unwrap(), None); + let request = r#"{"jsonrpc": "2.0", "method": "parity_getNewDappsWhitelist","params":[], "id": 1}"#; + let response = r#"{"jsonrpc":"2.0","result":null,"id":1}"#; + assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); +} + +#[test] +fn rpc_parity_recent_dapps() { + // given + let tester = setup(); + + // when + // trigger dapp usage + tester.accounts.note_dapp_used("dapp1".into()).unwrap(); + + // then + let request = r#"{"jsonrpc": "2.0", "method": "parity_listRecentDapps","params":[], "id": 1}"#; + let response = r#"{"jsonrpc":"2.0","result":["dapp1"],"id":1}"#; + assert_eq!(tester.io.handle_request_sync(request), Some(response.to_owned())); } #[test] diff --git a/rpc/src/v1/traits/parity_accounts.rs b/rpc/src/v1/traits/parity_accounts.rs index 6cfe1bf7b..95377540b 100644 --- a/rpc/src/v1/traits/parity_accounts.rs +++ b/rpc/src/v1/traits/parity_accounts.rs @@ -79,6 +79,24 @@ build_rpc_trait! { #[rpc(name = "parity_setDappsAddresses")] fn set_dapps_addresses(&self, DappId, Vec) -> Result; + /// Gets accounts exposed for particular dapp. + #[rpc(name = "parity_getDappsAddresses")] + fn dapps_addresses(&self, DappId) -> Result, Error>; + + /// Sets accounts exposed for new dapps. + /// `None` means that all accounts will be exposed. + #[rpc(name = "parity_setNewDappsWhitelist")] + fn set_new_dapps_whitelist(&self, Option>) -> Result; + + /// Gets accounts exposed for new dapps. + /// `None` means that all accounts will be exposed. + #[rpc(name = "parity_getNewDappsWhitelist")] + fn new_dapps_whitelist(&self) -> Result>, Error>; + + /// Sets accounts exposed for particular dapp. + #[rpc(name = "parity_listRecentDapps")] + fn recent_dapps(&self) -> Result, Error>; + /// Imports a number of Geth accounts, with the list provided as the argument. #[rpc(name = "parity_importGethAccounts")] fn import_geth_accounts(&self, Vec) -> Result, Error>; diff --git a/rpc/src/v1/types/dapp_id.rs b/rpc/src/v1/types/dapp_id.rs index 04aa80e3a..a21007892 100644 --- a/rpc/src/v1/types/dapp_id.rs +++ b/rpc/src/v1/types/dapp_id.rs @@ -26,6 +26,12 @@ impl Into for DappId { } } +impl From for DappId { + fn from(s: String) -> Self { + DappId(s) + } +} + #[cfg(test)] mod tests {