Fixing account naming (#1810)

* Fixing account naming

* Using geth format for files. Avoid re-importing existing keys with different names.

* Adding expect for time format

ethstore/src/account/safe_account.rs

@@ -15,7 +15,6 @@
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
 use std::ops::{Deref, DerefMut};
-use std::path::{PathBuf};
 use ethkey::{KeyPair, sign, Address, Secret, Signature, Message};
 use {json, Error, crypto};
 use crypto::Keccak256;
@@ -36,7 +35,7 @@ pub struct SafeAccount {
 	pub version: Version,
 	pub address: Address,
 	pub crypto: Crypto,
-	pub path: Option<PathBuf>,
+	pub filename: Option<String>,
 	pub name: String,
 	pub meta: String,
 }
@@ -63,20 +62,6 @@ impl Into<json::Crypto> for Crypto {
 	}
 }
 
-impl From<json::KeyFile> for SafeAccount {
-	fn from(json: json::KeyFile) -> Self {
-		SafeAccount {
-			id: json.id.into(),
-			version: json.version.into(),
-			address: json.address.into(),
-			crypto: json.crypto.into(),
-			path: None,
-			name: json.name.unwrap_or(String::new()),
-			meta: json.meta.unwrap_or("{}".to_owned()),
-		}
-	}
-}
-
 impl Into<json::KeyFile> for SafeAccount {
 	fn into(self) -> json::KeyFile {
 		json::KeyFile {
@@ -147,26 +132,32 @@ impl Crypto {
 }
 
 impl SafeAccount {
-	// DEPRECATED. use `create_with_name` instead
-	pub fn create(keypair: &KeyPair, id: [u8; 16], password: &str, iterations: u32, name: String, meta: String) -> Self {
+	pub fn create(
+		keypair: &KeyPair,
+		id: [u8; 16],
+		password: &str,
+		iterations: u32,
+		name: String,
+		meta: String
+	) -> Self {
 		SafeAccount {
 			id: id,
 			version: Version::V3,
 			crypto: Crypto::create(keypair.secret(), password, iterations),
 			address: keypair.address(),
-			path: None,
+			filename: None,
 			name: name,
 			meta: meta,
 		}
 	}
 
-	pub fn from_file(json: json::KeyFile, path: PathBuf) -> Self {
+	pub fn from_file(json: json::KeyFile, filename: String) -> Self {
 		SafeAccount {
 			id: json.id.into(),
 			version: json.version.into(),
 			address: json.address.into(),
 			crypto: json.crypto.into(),
-			path: Some(path),
+			filename: Some(filename),
 			name: json.name.unwrap_or(String::new()),
 			meta: json.meta.unwrap_or("{}".to_owned()),
 		}
@@ -184,7 +175,7 @@ impl SafeAccount {
 			version: self.version.clone(),
 			crypto: Crypto::create(&secret, new_password, iterations),
 			address: self.address.clone(),
-			path: self.path.clone(),
+			filename: self.filename.clone(),
 			name: self.name.clone(),
 			meta: self.meta.clone(),
 		};

ethstore/src/dir/disk.rs

@@ -77,7 +77,9 @@ impl DiskDirectory {
 			.map(json::KeyFile::load)
 			.zip(paths.into_iter())
 			.map(|(file, path)| match file {
-				Ok(file) => Ok((path, file.into())),
+				Ok(file) => Ok((path.clone(), SafeAccount::from_file(
+					file, path.file_name().and_then(|n| n.to_str()).expect("Keys have valid UTF8 names only.").to_owned()
+				))),
 				Err(err) => Err(Error::InvalidKeyFile(format!("{:?}: {}", path, err))),
 			})
 			.collect()
@@ -98,22 +100,26 @@ impl KeyDirectory for DiskDirectory {
 		let keyfile: json::KeyFile = account.clone().into();
 
 		// build file path
-		let mut account = account;
-		account.path = account.path.or_else(|| {
-			let mut keyfile_path = self.path.clone();
-			let timestamp = time::strftime("%Y-%m-%d_%H:%M:%S_%Z", &time::now()).unwrap_or("???".to_owned());
-			keyfile_path.push(format!("{}-{}.json", keyfile.id, timestamp));
-			Some(keyfile_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, account.address)
 		});
 
+		// update account filename
+		let mut account = account;
+		account.filename = Some(filename.clone());
+
 		{
+			// Path to keyfile
+			let mut keyfile_path = self.path.clone();
+			keyfile_path.push(filename.as_str());
+
 			// save the file
-			let path = account.path.as_ref().expect("build-file-path ensures is not None; qed");
-			let mut file = try!(fs::File::create(path));
+			let mut file = try!(fs::File::create(&keyfile_path));
 			try!(keyfile.write(&mut file).map_err(|e| Error::Custom(format!("{:?}", e))));
 
-			if let Err(_) = restrict_permissions_to_owner(path) {
-				fs::remove_file(path).expect("Expected to remove recently created file");
+			if let Err(_) = restrict_permissions_to_owner(keyfile_path.as_path()) {
+				fs::remove_file(keyfile_path).expect("Expected to remove recently created file");
 				return Err(Error::Io(io::Error::last_os_error()));
 			}
 		}
@@ -135,3 +141,34 @@ impl KeyDirectory for DiskDirectory {
 		}
 	}
 }
+
+
+#[cfg(test)]
+mod test {
+	use std::{env, fs};
+	use super::DiskDirectory;
+	use dir::KeyDirectory;
+	use account::SafeAccount;
+	use ethkey::{Random, Generator};
+
+	#[test]
+	fn should_create_new_account() {
+		// given
+		let dir = env::temp_dir();
+		let keypair = Random.generate().unwrap();
+		let password = "hello world";
+		let directory = DiskDirectory::create(dir.clone()).unwrap();
+
+		// when
+		let account = SafeAccount::create(&keypair, [0u8; 16], password, 1024, "Test".to_owned(), "{}".to_owned());
+		let res = directory.insert(account);
+
+
+		// then
+		assert!(res.is_ok(), "Should save account succesfuly.");
+		assert!(res.unwrap().filename.is_some(), "Filename has been assigned.");
+
+		// cleanup
+		let _ = fs::remove_dir_all(dir);
+	}
+}

ethstore/src/import.rs

@@ -14,15 +14,20 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
+use std::collections::HashSet;
 use ethkey::Address;
 use dir::KeyDirectory;
 use Error;
 
 pub fn import_accounts(src: &KeyDirectory, dst: &KeyDirectory) -> Result<Vec<Address>, Error> {
 	let accounts = try!(src.load());
-	accounts.into_iter().map(|a| {
-		let address = a.address.clone();
-		try!(dst.insert(a));
-		Ok(address)
-	}).collect()
+	let existing_accounts = try!(dst.load()).into_iter().map(|a| a.address).collect::<HashSet<_>>();
+
+	accounts.into_iter()
+		.filter(|a| !existing_accounts.contains(&a.address))
+		.map(|a| {
+			let address = a.address.clone();
+			try!(dst.insert(a));
+			Ok(address)
+		}).collect()
 }

parity/account.rs

@@ -47,21 +47,25 @@ pub fn execute(cmd: AccountCmd) -> Result<String, String> {
 	}
 }
 
+fn keys_dir(path: String) -> Result<DiskDirectory, String> {
+	DiskDirectory::create(path).map_err(|e| format!("Could not open keys directory: {}", e))
+}
+
 fn new(n: NewAccount) -> Result<String, String> {
 	let password: String = match n.password_file {
 		Some(file) => try!(password_from_file(file)),
 		None => try!(password_prompt()),
 	};
 
-	let dir = Box::new(DiskDirectory::create(n.path).unwrap());
+	let dir = Box::new(try!(keys_dir(n.path)));
 	let secret_store = Box::new(EthStore::open_with_iterations(dir, n.iterations).unwrap());
 	let acc_provider = AccountProvider::new(secret_store);
-	let new_account = acc_provider.new_account(&password).unwrap();
+	let new_account = try!(acc_provider.new_account(&password).map_err(|e| format!("Could not create new account: {}", e)));
 	Ok(format!("{:?}", new_account))
 }
 
 fn list(path: String) -> Result<String, String> {
-	let dir = Box::new(DiskDirectory::create(path).unwrap());
+	let dir = Box::new(try!(keys_dir(path)));
 	let secret_store = Box::new(EthStore::open(dir).unwrap());
 	let acc_provider = AccountProvider::new(secret_store);
 	let accounts = acc_provider.accounts();
@@ -74,7 +78,7 @@ fn list(path: String) -> Result<String, String> {
 }
 
 fn import(i: ImportAccounts) -> Result<String, String> {
-	let to = DiskDirectory::create(i.to).unwrap();
+	let to = try!(keys_dir(i.to));
 	let mut imported = 0;
 	for path in &i.from {
 		let from = DiskDirectory::at(path);

parity/run.rs

@@ -298,7 +298,7 @@ fn prepare_account_provider(dirs: &Directories, cfg: AccountsConfig) -> Result<A
 		};
 
 		let from = GethDirectory::open(t);
-		let to = DiskDirectory::create(dirs.keys.clone()).unwrap();
+		let to = try!(DiskDirectory::create(dirs.keys.clone()).map_err(|e| format!("Could not open keys directory: {}", e)));
 		match import_accounts(&from, &to) {
 			Ok(_) => {}
 			Err(Error::Io(ref io_err)) if io_err.kind() == ErrorKind::NotFound => {}
@@ -306,8 +306,10 @@ fn prepare_account_provider(dirs: &Directories, cfg: AccountsConfig) -> Result<A
 		}
 	}
 
-	let dir = Box::new(DiskDirectory::create(dirs.keys.clone()).unwrap());
-	let account_service = AccountProvider::new(Box::new(EthStore::open_with_iterations(dir, cfg.iterations).unwrap()));
+	let dir = Box::new(try!(DiskDirectory::create(dirs.keys.clone()).map_err(|e| format!("Could not open keys directory: {}", e))));
+	let account_service = AccountProvider::new(Box::new(
+		try!(EthStore::open_with_iterations(dir, cfg.iterations).map_err(|e| format!("Could not open keys directory: {}", e)))
+	));
 
 	for a in cfg.unlocked_accounts {
 		if passwords.iter().find(|p| account_service.unlock_account_permanently(a, (*p).clone()).is_ok()).is_none() {