Unlock account with timeout for geth compatibility (#1854)

* Unlock account with timeout for geth compatibility

* Fixed test

ethcore/src/account_provider.rs

@@ -18,7 +18,8 @@
 
 use std::fmt;
 use std::collections::HashMap;
-use util::{Address as H160, H256, H520, RwLock};
+use std::time::{Instant, Duration};
+use util::{Address as H160, H256, H520, Mutex, RwLock};
 use ethstore::{SecretStore, Error as SSError, SafeAccount, EthStore};
 use ethstore::dir::{KeyDirectory};
 use ethstore::ethkey::{Address as SSAddress, Message as SSMessage, Secret as SSSecret, Random, Generator};
@@ -32,6 +33,8 @@ enum Unlock {
 	/// Account unlocked permantently can always sign message.
 	/// Use with caution.
 	Perm,
+	/// Account unlocked with a timeout
+	Timed((Instant, u32)),
 }
 
 /// Data associated with account.
@@ -131,7 +134,7 @@ impl KeyDirectory for NullDir {
 /// Account management.
 /// Responsible for unlocking accounts.
 pub struct AccountProvider {
-	unlocked: RwLock<HashMap<SSAddress, AccountData>>,
+	unlocked: Mutex<HashMap<SSAddress, AccountData>>,
 	sstore: Box<SecretStore>,
 }
 
@@ -160,7 +163,7 @@ impl AccountProvider {
 	/// Creates new account provider.
 	pub fn new(sstore: Box<SecretStore>) -> Self {
 		AccountProvider {
-			unlocked: RwLock::new(HashMap::new()),
+			unlocked: Mutex::new(HashMap::new()),
 			sstore: sstore,
 		}
 	}
@@ -168,7 +171,7 @@ impl AccountProvider {
 	/// Creates not disk backed provider.
 	pub fn transient_provider() -> Self {
 		AccountProvider {
-			unlocked: RwLock::new(HashMap::new()),
+			unlocked: Mutex::new(HashMap::new()),
 			sstore: Box::new(EthStore::open(Box::new(NullDir::default())).unwrap())
 		}
 	}
@@ -236,12 +239,10 @@ impl AccountProvider {
 		let _ = try!(self.sstore.sign(&account, &password, &Default::default()));
 
 		// check if account is already unlocked pernamently, if it is, do nothing
-		{
+		let mut unlocked = self.unlocked.lock();
-			let unlocked = self.unlocked.read();
+		if let Some(data) = unlocked.get(&account) {
-			if let Some(data) = unlocked.get(&account) {
+			if let Unlock::Perm = data.unlock {
-				if let Unlock::Perm = data.unlock {
+				return Ok(())
-					return Ok(())
-				}
 			}
 		}
 
@@ -250,7 +251,6 @@ impl AccountProvider {
 			password: password,
 		};
 
-		let mut unlocked = self.unlocked.write();
 		unlocked.insert(account, data);
 		Ok(())
 	}
@@ -265,10 +265,15 @@ impl AccountProvider {
 		self.unlock_account(account, password, Unlock::Temp)
 	}
 
+	/// Unlocks account temporarily with a timeout.
+	pub fn unlock_account_timed<A>(&self, account: A, password: String, duration_ms: u32) -> Result<(), Error> where Address: From<A> {
+		self.unlock_account(account, password, Unlock::Timed((Instant::now(), duration_ms)))
+	}
+
 	/// Checks if given account is unlocked
 	pub fn is_unlocked<A>(&self, account: A) -> bool where Address: From<A> {
 		let account = Address::from(account).into();
-		let unlocked = self.unlocked.read();
+		let unlocked = self.unlocked.lock();
 		unlocked.get(&account).is_some()
 	}
 
@@ -278,15 +283,20 @@ impl AccountProvider {
 		let message = Message::from(message).into();
 
 		let data = {
-			let unlocked = self.unlocked.read();
+			let mut unlocked = self.unlocked.lock();
-			try!(unlocked.get(&account).ok_or(Error::NotUnlocked)).clone()
+			let data = try!(unlocked.get(&account).ok_or(Error::NotUnlocked)).clone();
+			if let Unlock::Temp = data.unlock {
+				unlocked.remove(&account).expect("data exists: so key must exist: qed");
+			}
+			if let Unlock::Timed((ref start, ref duration)) = data.unlock {
+				if start.elapsed() > Duration::from_millis(*duration as u64) {
+					unlocked.remove(&account).expect("data exists: so key must exist: qed");
+					return Err(Error::NotUnlocked);
+				}
+			}
+			data
 		};
 
-		if let Unlock::Temp = data.unlock {
-			let mut unlocked = self.unlocked.write();
-			unlocked.remove(&account).expect("data exists: so key must exist: qed");
-		}
-
 		let signature = try!(self.sstore.sign(&account, &data.password, &message));
 		Ok(H520(signature.into()))
 	}
@@ -304,6 +314,7 @@ impl AccountProvider {
 mod tests {
 	use super::AccountProvider;
 	use ethstore::ethkey::{Generator, Random};
+	use std::time::Duration;
 
 	#[test]
 	fn unlock_account_temp() {
@@ -329,4 +340,16 @@ mod tests {
 		assert!(ap.sign(kp.address(), [0u8; 32]).is_ok());
 		assert!(ap.sign(kp.address(), [0u8; 32]).is_ok());
 	}
+
+	#[test]
+	fn unlock_account_timer() {
+		let kp = Random.generate().unwrap();
+		let ap = AccountProvider::transient_provider();
+		assert!(ap.insert_account(kp.secret().clone(), "test").is_ok());
+		assert!(ap.unlock_account_timed(kp.address(), "test1".into(), 2000).is_err());
+		assert!(ap.unlock_account_timed(kp.address(), "test".into(), 2000).is_ok());
+		assert!(ap.sign(kp.address(), [0u8; 32]).is_ok());
+		::std::thread::sleep(Duration::from_millis(2000));
+		assert!(ap.sign(kp.address(), [0u8; 32]).is_err());
+	}
 }

parity/rpc_apis.rs

@@ -179,7 +179,7 @@ pub fn setup_rpc<T: Extendable>(server: T, deps: Arc<Dependencies>, apis: ApiSet
 				}
 			},
 			Api::Personal => {
-				server.add_delegate(PersonalClient::new(&deps.secret_store, &deps.client, &deps.miner, deps.signer_port).to_delegate());
+				server.add_delegate(PersonalClient::new(&deps.secret_store, &deps.client, &deps.miner, deps.signer_port, deps.geth_compatibility).to_delegate());
 			},
 			Api::Signer => {
 				server.add_delegate(SignerClient::new(&deps.secret_store, &deps.client, &deps.miner, &deps.signer_queue).to_delegate());

rpc/src/v1/impls/personal.rs

@@ -33,16 +33,18 @@ pub struct PersonalClient<C, M> where C: MiningBlockChainClient, M: MinerService
 	client: Weak<C>,
 	miner: Weak<M>,
 	signer_port: Option<u16>,
+	allow_perm_unlock: bool,
 }
 
 impl<C, M> PersonalClient<C, M> where C: MiningBlockChainClient, M: MinerService {
 	/// Creates new PersonalClient
-	pub fn new(store: &Arc<AccountProvider>, client: &Arc<C>, miner: &Arc<M>, signer_port: Option<u16>) -> Self {
+	pub fn new(store: &Arc<AccountProvider>, client: &Arc<C>, miner: &Arc<M>, signer_port: Option<u16>, allow_perm_unlock: bool) -> Self {
 		PersonalClient {
 			accounts: Arc::downgrade(store),
 			client: Arc::downgrade(client),
 			miner: Arc::downgrade(miner),
 			signer_port: signer_port,
+			allow_perm_unlock: allow_perm_unlock,
 		}
 	}
 
@@ -89,11 +91,17 @@ impl<C: 'static, M: 'static> Personal for PersonalClient<C, M> where C: MiningBl
 
 	fn unlock_account(&self, params: Params) -> Result<Value, Error> {
 		try!(self.active());
-		from_params::<(RpcH160, String, u64)>(params).and_then(
+		from_params::<(RpcH160, String, Option<u64>)>(params).and_then(
-			|(account, account_pass, _)|{
+			|(account, account_pass, duration)|{
 				let account: Address = account.into();
 				let store = take_weak!(self.accounts);
-				match store.unlock_account_temporarily(account, account_pass) {
+				let r = match (self.allow_perm_unlock, duration) {
+					(false, _) => store.unlock_account_temporarily(account, account_pass),
+					(true, Some(0)) => store.unlock_account_permanently(account, account_pass),
+					(true, Some(d)) => store.unlock_account_timed(account, account_pass, d as u32 * 1000),
+					(true, None) => store.unlock_account_timed(account, account_pass, 300_000),
+				};
+				match r {
 					Ok(_) => Ok(Value::Bool(true)),
 					Err(_) => Ok(Value::Bool(false)),
 				}

rpc/src/v1/tests/mocked/personal.rs

@@ -50,7 +50,7 @@ fn setup(signer: Option<u16>) -> PersonalTester {
 	let accounts = accounts_provider();
 	let client = blockchain_client();
 	let miner = miner_service();
-	let personal = PersonalClient::new(&accounts, &client, &miner, signer);
+	let personal = PersonalClient::new(&accounts, &client, &miner, signer, false);
 
 	let io = IoHandler::new();
 	io.add_delegate(personal.to_delegate());