Fix base-path handling in key insert (#7775)

This fixes the handling of base-path when using `key insert`. Before
the base-path wasn't setup correctly, as done when starting a node. This
resulted in putting the keys into the wrong directory. This pr fixes
this by creating the correct base-path/config dir for the keystore.
Besides that it also removes the insert command from `subkey` as it
doesn't make that much sense. If requested, we could bring it back later.

substrate/client/cli/src/commands/insert.rs

@@ -1,94 +0,0 @@
-// This file is part of Substrate.
-
-// Copyright (C) 2020 Parity Technologies (UK) Ltd.
-// SPDX-License-Identifier: Apache-2.0
-
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// 	http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-//! Implementation of the `insert` subcommand
-
-use crate::{Error, KeystoreParams, CryptoSchemeFlag, SharedParams, utils, with_crypto_scheme};
-use std::sync::Arc;
-use structopt::StructOpt;
-use sp_core::crypto::KeyTypeId;
-use sp_keystore::{SyncCryptoStorePtr, SyncCryptoStore};
-use std::convert::TryFrom;
-use sc_service::config::KeystoreConfig;
-use sc_keystore::LocalKeystore;
-use sp_core::crypto::SecretString;
-
-/// The `insert` command
-#[derive(Debug, StructOpt)]
-#[structopt(
-	name = "insert",
-	about = "Insert a key to the keystore of a node."
-)]
-pub struct InsertCmd {
-	/// The secret key URI.
-	/// If the value is a file, the file content is used as URI.
-	/// If not given, you will be prompted for the URI.
-	#[structopt(long)]
-	suri: Option<String>,
-
-	/// Key type, examples: "gran", or "imon"
-	#[structopt(long)]
-	key_type: String,
-
-	#[allow(missing_docs)]
-	#[structopt(flatten)]
-	pub shared_params: SharedParams,
-
-	#[allow(missing_docs)]
-	#[structopt(flatten)]
-	pub keystore_params: KeystoreParams,
-
-	#[allow(missing_docs)]
-	#[structopt(flatten)]
-	pub crypto_scheme: CryptoSchemeFlag,
-}
-
-impl InsertCmd {
-	/// Run the command
-	pub fn run(&self) -> Result<(), Error> {
-		let suri = utils::read_uri(self.suri.as_ref())?;
-		let base_path = self.shared_params.base_path.as_ref()
-			.ok_or_else(|| Error::MissingBasePath)?;
-
-		let (keystore, public) = match self.keystore_params.keystore_config(base_path)? {
-			(_, KeystoreConfig::Path { path, password }) => {
-				let public = with_crypto_scheme!(
-					self.crypto_scheme.scheme,
-					to_vec(&suri, password.clone())
-				)?;
-				let keystore: SyncCryptoStorePtr = Arc::new(LocalKeystore::open(path, password)?);
-				(keystore, public)
-			},
-			_ => unreachable!("keystore_config always returns path and password; qed")
-		};
-
-		let key_type = KeyTypeId::try_from(self.key_type.as_str())
-			.map_err(|_e| {
-				Error::KeyTypeInvalid
-			})?;
-
-		SyncCryptoStore::insert_unknown(&*keystore, key_type, &suri, &public[..])
-			.map_err(|_| Error::KeyStoreOperation)?;
-
-		Ok(())
-	}
-}
-
-fn to_vec<P: sp_core::Pair>(uri: &str, pass: Option<SecretString>) -> Result<Vec<u8>, Error> {
-	let p = utils::pair_from_suri::<P>(uri, pass)?;
-	Ok(p.public().as_ref().to_vec())
-}

substrate/client/cli/src/commands/insert_key.rs

@@ -0,0 +1,173 @@
+// This file is part of Substrate.
+
+// Copyright (C) 2020 Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Implementation of the `insert` subcommand
+
+use crate::{
+	Error, KeystoreParams, CryptoSchemeFlag, SharedParams, utils, with_crypto_scheme,
+	SubstrateCli,
+};
+use std::{sync::Arc, convert::TryFrom};
+use structopt::StructOpt;
+use sp_core::{crypto::KeyTypeId, crypto::SecretString};
+use sp_keystore::{SyncCryptoStorePtr, SyncCryptoStore};
+use sc_keystore::LocalKeystore;
+use sc_service::config::{KeystoreConfig, BasePath};
+
+/// The `insert` command
+#[derive(Debug, StructOpt)]
+#[structopt(
+	name = "insert",
+	about = "Insert a key to the keystore of a node."
+)]
+pub struct InsertKeyCmd {
+	/// The secret key URI.
+	/// If the value is a file, the file content is used as URI.
+	/// If not given, you will be prompted for the URI.
+	#[structopt(long)]
+	suri: Option<String>,
+
+	/// Key type, examples: "gran", or "imon"
+	#[structopt(long)]
+	key_type: String,
+
+	#[allow(missing_docs)]
+	#[structopt(flatten)]
+	pub shared_params: SharedParams,
+
+	#[allow(missing_docs)]
+	#[structopt(flatten)]
+	pub keystore_params: KeystoreParams,
+
+	#[allow(missing_docs)]
+	#[structopt(flatten)]
+	pub crypto_scheme: CryptoSchemeFlag,
+}
+
+impl InsertKeyCmd {
+	/// Run the command
+	pub fn run<C: SubstrateCli>(&self, cli: &C) -> Result<(), Error> {
+		let suri = utils::read_uri(self.suri.as_ref())?;
+		let base_path = self.shared_params
+			.base_path()
+			.unwrap_or_else(|| BasePath::from_project("", "", &C::executable_name()));
+		let chain_id = self.shared_params.chain_id(self.shared_params.is_dev());
+		let chain_spec = cli.load_spec(&chain_id)?;
+		let config_dir = base_path.config_dir(chain_spec.id());
+
+		let (keystore, public) = match self.keystore_params.keystore_config(&config_dir)? {
+			(_, KeystoreConfig::Path { path, password }) => {
+				let public = with_crypto_scheme!(
+					self.crypto_scheme.scheme,
+					to_vec(&suri, password.clone())
+				)?;
+				let keystore: SyncCryptoStorePtr = Arc::new(LocalKeystore::open(path, password)?);
+				(keystore, public)
+			},
+			_ => unreachable!("keystore_config always returns path and password; qed")
+		};
+
+		let key_type = KeyTypeId::try_from(self.key_type.as_str()).map_err(|_| Error::KeyTypeInvalid)?;
+
+		SyncCryptoStore::insert_unknown(&*keystore, key_type, &suri, &public[..])
+			.map_err(|_| Error::KeyStoreOperation)?;
+
+		Ok(())
+	}
+}
+
+fn to_vec<P: sp_core::Pair>(uri: &str, pass: Option<SecretString>) -> Result<Vec<u8>, Error> {
+	let p = utils::pair_from_suri::<P>(uri, pass)?;
+	Ok(p.public().as_ref().to_vec())
+}
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+	use structopt::StructOpt;
+	use tempfile::TempDir;
+	use sp_core::{sr25519::Pair, Pair as _, Public};
+	use sc_service::{ChainSpec, GenericChainSpec, ChainType, NoExtension};
+
+	struct Cli;
+
+	impl SubstrateCli for Cli {
+		fn impl_name() -> String {
+			"test".into()
+		}
+
+		fn impl_version() -> String {
+			"2.0".into()
+		}
+
+		fn description() -> String {
+			"test".into()
+		}
+
+		fn support_url() -> String {
+			"test.test".into()
+		}
+
+		fn copyright_start_year() -> i32 {
+			2020
+		}
+
+		fn author() -> String {
+			"test".into()
+		}
+
+		fn native_runtime_version(_: &Box<dyn ChainSpec>) -> &'static sp_version::RuntimeVersion {
+			unimplemented!("Not required in tests")
+		}
+
+		fn load_spec(&self, _: &str) -> std::result::Result<Box<dyn ChainSpec>, String> {
+			Ok(
+				Box::new(
+					GenericChainSpec::from_genesis(
+						"test",
+						"test_id",
+						ChainType::Development,
+						|| unimplemented!("Not required in tests"),
+						Vec::new(),
+						None,
+						None,
+						None,
+						NoExtension::None,
+					),
+				),
+			)
+		}
+	}
+
+	#[test]
+	fn insert_with_custom_base_path() {
+		let path = TempDir::new().unwrap();
+		let path_str = format!("{}", path.path().display());
+		let (key, uri, _) = Pair::generate_with_phrase(None);
+
+		let inspect = InsertKeyCmd::from_iter(
+			&["insert-key", "-d", &path_str, "--key-type", "test", "--suri", &uri],
+		);
+		assert!(inspect.run(&Cli).is_ok());
+
+		let keystore = LocalKeystore::open(
+			path.path().join("chains").join("test_id").join("keystore"),
+			None,
+		).unwrap();
+		assert!(keystore.has_keys(&[(key.public().to_raw_vec(), KeyTypeId(*b"test"))]));
+	}
+}

substrate/client/cli/src/commands/key.rs

@@ -17,11 +17,11 @@
 
 //! Key related CLI utilities
 
-use crate::Error;
+use crate::{Error, SubstrateCli};
 use structopt::StructOpt;
 
 use super::{
-	insert::InsertCmd,
+	insert_key::InsertKeyCmd,
 	inspect_key::InspectKeyCmd,
 	generate::GenerateCmd,
 	inspect_node_key::InspectNodeKeyCmd,
@@ -45,17 +45,17 @@ pub enum KeySubcommand {
 	InspectNodeKey(InspectNodeKeyCmd),
 
 	/// Insert a key to the keystore of a node.
-	Insert(InsertCmd),
+	Insert(InsertKeyCmd),
 }
 
 impl KeySubcommand {
 	/// run the key subcommands
-	pub fn run(&self) -> Result<(), Error> {
+	pub fn run<C: SubstrateCli>(&self, cli: &C) -> Result<(), Error> {
 		match self {
 			KeySubcommand::GenerateNodeKey(cmd) => cmd.run(),
 			KeySubcommand::Generate(cmd) => cmd.run(),
 			KeySubcommand::InspectKey(cmd) => cmd.run(),
-			KeySubcommand::Insert(cmd) => cmd.run(),
+			KeySubcommand::Insert(cmd) => cmd.run(cli),
 			KeySubcommand::InspectNodeKey(cmd) => cmd.run(),
 		}
 	}

substrate/client/cli/src/commands/mod.rs

@@ -28,7 +28,7 @@ mod revert_cmd;
 mod run_cmd;
 mod generate_node_key;
 mod generate;
-mod insert;
+mod insert_key;
 mod inspect_node_key;
 mod inspect_key;
 mod key;
@@ -43,7 +43,7 @@ pub use self::{
 	purge_chain_cmd::PurgeChainCmd,
 	sign::SignCmd,
 	generate::GenerateCmd,
-	insert::InsertCmd,
+	insert_key::InsertKeyCmd,
 	inspect_key::InspectKeyCmd,
 	generate_node_key::GenerateNodeKeyCmd,
 	inspect_node_key::InspectNodeKeyCmd,

substrate/client/cli/src/config.rs

@@ -186,11 +186,11 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
 
 	/// Get the keystore configuration.
 	///
-	/// Bu default this is retrieved from `KeystoreParams` if it is available. Otherwise it uses
+	/// By default this is retrieved from `KeystoreParams` if it is available. Otherwise it uses
 	/// `KeystoreConfig::InMemory`.
-	fn keystore_config(&self, base_path: &PathBuf) -> Result<(Option<String>, KeystoreConfig)> {
+	fn keystore_config(&self, config_dir: &PathBuf) -> Result<(Option<String>, KeystoreConfig)> {
 		self.keystore_params()
-			.map(|x| x.keystore_config(base_path))
+			.map(|x| x.keystore_config(config_dir))
 			.unwrap_or_else(|| Ok((None, KeystoreConfig::InMemory)))
 	}
 
@@ -454,15 +454,11 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
 	) -> Result<Configuration> {
 		let is_dev = self.is_dev()?;
 		let chain_id = self.chain_id(is_dev)?;
-		let chain_spec = cli.load_spec(chain_id.as_str())?;
+		let chain_spec = cli.load_spec(&chain_id)?;
 		let base_path = self
 			.base_path()?
 			.unwrap_or_else(|| BasePath::from_project("", "", &C::executable_name()));
-		let config_dir = base_path
-			.path()
-			.to_path_buf()
-			.join("chains")
-			.join(chain_spec.id());
+		let config_dir = base_path.config_dir(chain_spec.id());
 		let net_config_dir = config_dir.join(DEFAULT_NETWORK_CONFIG_PATH);
 		let client_id = C::client_id();
 		let database_cache_size = self.database_cache_size()?.unwrap_or(128);

substrate/client/cli/src/error.rs

@@ -59,9 +59,6 @@ pub enum Error {
 		expected: usize,
 	},
 
-	#[error("The base path is missing, please provide one")]
-	MissingBasePath,
-
 	#[error("Unknown key type, must be a known 4-character sequence")]
 	KeyTypeInvalid,
 

substrate/client/cli/src/lib.rs

@@ -112,10 +112,7 @@ pub trait SubstrateCli: Sized {
 	///
 	/// Gets the struct from the command line arguments. Print the
 	/// error message and quit the program in case of failure.
-	fn from_args() -> Self
-	where
-		Self: StructOpt + Sized,
-	{
+	fn from_args() -> Self where Self: StructOpt + Sized {
 		<Self as SubstrateCli>::from_iter(&mut std::env::args_os())
 	}
 

substrate/client/cli/src/params/keystore_params.rs

@@ -18,8 +18,7 @@
 
 use crate::error::Result;
 use sc_service::config::KeystoreConfig;
-use std::fs;
-use std::path::PathBuf;
+use std::{fs, path::{PathBuf, Path}};
 use structopt::StructOpt;
 use crate::error;
 use sp_core::crypto::SecretString;
@@ -33,6 +32,7 @@ pub struct KeystoreParams {
 	/// Specify custom URIs to connect to for keystore-services
 	#[structopt(long = "keystore-uri")]
 	pub keystore_uri: Option<String>,
+
 	/// Specify custom keystore path.
 	#[structopt(long = "keystore-path", value_name = "PATH", parse(from_os_str))]
 	pub keystore_path: Option<PathBuf>,
@@ -64,15 +64,14 @@ pub struct KeystoreParams {
 
 /// Parse a sercret string, returning a displayable error.
 pub fn secret_string_from_str(s: &str) -> std::result::Result<SecretString, String> {
-	Ok(std::str::FromStr::from_str(s)
-		.map_err(|_e| "Could not get SecretString".to_string())?)
+	std::str::FromStr::from_str(s).map_err(|_| "Could not get SecretString".to_string())
 }
 
 impl KeystoreParams {
 	/// Get the keystore configuration for the parameters
-	/// returns a vector of remote-urls and the local Keystore configuration
-	pub fn keystore_config(&self, base_path: &PathBuf) -> Result<(Option<String>, KeystoreConfig)> {
-
+	///
+	/// Returns a vector of remote-urls and the local Keystore configuration
+	pub fn keystore_config(&self, config_dir: &Path) -> Result<(Option<String>, KeystoreConfig)> {
 		let password = if self.password_interactive {
 			#[cfg(not(target_os = "unknown"))]
 			{
@@ -92,7 +91,7 @@ impl KeystoreParams {
 		let path = self
 			.keystore_path
 			.clone()
-			.unwrap_or_else(|| base_path.join(DEFAULT_KEYSTORE_CONFIG_PATH));
+			.unwrap_or_else(|| config_dir.join(DEFAULT_KEYSTORE_CONFIG_PATH));
 
 		Ok((self.keystore_uri.clone(), KeystoreConfig::Path { path, password }))
 	}