From d8723149ac27f3279544f879159a3e55da0a39cc Mon Sep 17 00:00:00 2001 From: etorreborre Date: Wed, 29 Nov 2023 09:13:34 +0100 Subject: [PATCH 1/9] fix(rust): fix the passing of space name and add a better error for decoding errors for identifiers --- .../ockam_app_lib/src/enroll/enroll_user.rs | 2 +- .../ockam/ockam_command/src/enroll/command.rs | 2 +- .../rust/ockam/ockam_identity/src/error.rs | 3 +- .../src/models/utils/identifiers.rs | 34 ++++++++++--------- 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/implementations/rust/ockam/ockam_app_lib/src/enroll/enroll_user.rs b/implementations/rust/ockam/ockam_app_lib/src/enroll/enroll_user.rs index cd497b3afaf..ed030ae6367 100644 --- a/implementations/rust/ockam/ockam_app_lib/src/enroll/enroll_user.rs +++ b/implementations/rust/ockam/ockam_app_lib/src/enroll/enroll_user.rs @@ -190,7 +190,7 @@ impl AppState { }); let ctx = &self.context(); let project = node_manager - .create_project(ctx, &space.id, PROJECT_NAME, vec![]) + .create_project(ctx, &space.name, PROJECT_NAME, vec![]) .await?; node_manager .wait_until_project_is_ready(ctx, project) diff --git a/implementations/rust/ockam/ockam_command/src/enroll/command.rs b/implementations/rust/ockam/ockam_command/src/enroll/command.rs index c46de152315..cd53f5ee32c 100644 --- a/implementations/rust/ockam/ockam_command/src/enroll/command.rs +++ b/implementations/rust/ockam/ockam_command/src/enroll/command.rs @@ -307,7 +307,7 @@ async fn get_user_project( let project_name = "default".to_string(); let get_project = async { let project = node - .create_project(ctx, &space.id, &project_name, vec![]) + .create_project(ctx, &space.name, &project_name, vec![]) .await?; *is_finished.lock().await = true; Ok(project) diff --git a/implementations/rust/ockam/ockam_identity/src/error.rs b/implementations/rust/ockam/ockam_identity/src/error.rs index 9d605c48253..5973f0d9e5e 100644 --- a/implementations/rust/ockam/ockam_identity/src/error.rs +++ b/implementations/rust/ockam/ockam_identity/src/error.rs @@ -4,6 +4,7 @@ use ockam_core::{ }; /// Identity crate error +#[repr(u8)] #[derive(Clone, Debug)] pub enum IdentityError { /// Invalid key type @@ -11,7 +12,7 @@ pub enum IdentityError { /// Invalid Key Data InvalidKeyData, /// Invalid Identifier format - InvalidIdentifier, + InvalidIdentifier(String), /// Identity Change History is empty EmptyIdentity, /// Identity Verification Failed diff --git a/implementations/rust/ockam/ockam_identity/src/models/utils/identifiers.rs b/implementations/rust/ockam/ockam_identity/src/models/utils/identifiers.rs index fc3e4494a78..a1a2219d8a7 100644 --- a/implementations/rust/ockam/ockam_identity/src/models/utils/identifiers.rs +++ b/implementations/rust/ockam/ockam_identity/src/models/utils/identifiers.rs @@ -1,18 +1,19 @@ +use core::fmt::{Display, Formatter}; +use core::str::FromStr; + +use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; + +use ockam_core::{Error, Result}; use ockam_core::compat::{string::String, vec::Vec}; use ockam_core::env::FromString; -use ockam_core::{Error, Result}; use crate::{Identifier, IdentityError}; - -use crate::models::{ChangeHash, CHANGE_HASH_LEN, IDENTIFIER_LEN}; -use core::fmt::{Display, Formatter}; -use core::str::FromStr; -use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; +use crate::models::{CHANGE_HASH_LEN, ChangeHash, IDENTIFIER_LEN}; impl Serialize for Identifier { fn serialize(&self, serializer: S) -> core::result::Result - where - S: Serializer, + where + S: Serializer, { serializer.serialize_str(&String::from(self)) } @@ -20,8 +21,8 @@ impl Serialize for Identifier { impl<'de> Deserialize<'de> for Identifier { fn deserialize(deserializer: D) -> core::result::Result - where - D: Deserializer<'de>, + where + D: Deserializer<'de>, { let str: String = Deserialize::deserialize(deserializer)?; @@ -60,10 +61,10 @@ impl TryFrom<&str> for Identifier { if let Ok(data) = hex::decode(value) { data.try_into() } else { - Err(IdentityError::InvalidIdentifier.into()) + Err(IdentityError::InvalidIdentifier(value.into()).into()) } } else { - Err(IdentityError::InvalidIdentifier.into()) + Err(IdentityError::InvalidIdentifier(value.into()).into()) } } } @@ -75,7 +76,7 @@ impl TryFrom<&[u8]> for Identifier { if let Ok(value) = <[u8; IDENTIFIER_LEN]>::try_from(value) { Ok(Self(value)) } else { - Err(IdentityError::InvalidIdentifier.into()) + Err(IdentityError::InvalidIdentifier(hex::encode(value)).into()) } } } @@ -142,7 +143,7 @@ impl TryFrom<&str> for ChangeHash { if let Ok(data) = hex::decode(value) { data.try_into() } else { - Err(IdentityError::InvalidIdentifier.into()) + Err(IdentityError::InvalidIdentifier(value.into()).into()) } } } @@ -154,7 +155,7 @@ impl TryFrom<&[u8]> for ChangeHash { if let Ok(value) = <[u8; CHANGE_HASH_LEN]>::try_from(value) { Ok(Self(value)) } else { - Err(IdentityError::InvalidIdentifier.into()) + Err(IdentityError::InvalidIdentifier(hex::encode(value)).into()) } } } @@ -197,8 +198,9 @@ impl AsRef<[u8]> for ChangeHash { #[cfg(test)] mod test { + use quickcheck::{Arbitrary, Gen, quickcheck}; + use super::*; - use quickcheck::{quickcheck, Arbitrary, Gen}; impl Arbitrary for Identifier { fn arbitrary(g: &mut Gen) -> Self { From d3b241d8ed9ad4417bf524038324e76fb933391a Mon Sep 17 00:00:00 2001 From: etorreborre Date: Wed, 29 Nov 2023 09:30:12 +0100 Subject: [PATCH 2/9] fix(rust): create the authority identity if it doesn't exist --- .../ockam_command/src/authority/create.rs | 35 ++++++++++--------- .../ockam_command/tests/bats/authority.bats | 16 +++++++++ 2 files changed, 34 insertions(+), 17 deletions(-) diff --git a/implementations/rust/ockam/ockam_command/src/authority/create.rs b/implementations/rust/ockam/ockam_command/src/authority/create.rs index 40b628b161a..f95ae94599c 100644 --- a/implementations/rust/ockam/ockam_command/src/authority/create.rs +++ b/implementations/rust/ockam/ockam_command/src/authority/create.rs @@ -4,11 +4,11 @@ use std::process; use clap::ArgGroup; use clap::Args; -use miette::{miette, IntoDiagnostic}; +use miette::{IntoDiagnostic, miette}; use serde::{Deserialize, Serialize}; -use ockam::identity::{AttributesEntry, Identifier}; use ockam::Context; +use ockam::identity::{AttributesEntry, Identifier}; use ockam_api::authority_node; use ockam_api::authority_node::{OktaConfiguration, TrustedIdentity}; use ockam_api::bootstrapped_identities_store::PreTrustedIdentities; @@ -17,11 +17,11 @@ use ockam_api::nodes::service::default_address::DefaultAddress; use ockam_core::compat::collections::HashMap; use ockam_core::compat::fmt; +use crate::{CommandGlobalOpts, docs, Result}; use crate::node::util::run_ockam; -use crate::util::parsers::internet_address_parser; use crate::util::{embedded_node_that_is_not_stopped, exitcode}; use crate::util::{local_cmd, node_rpc}; -use crate::{docs, CommandGlobalOpts, Result}; +use crate::util::parsers::internet_address_parser; const LONG_ABOUT: &str = include_str!("./static/create/long_about.txt"); const PREVIEW_TAG: &str = include_str!("../static/preview_tag.txt"); @@ -110,6 +110,13 @@ async fn spawn_background_node( opts: &CommandGlobalOpts, cmd: &CreateCommand, ) -> miette::Result<()> { + // Create the authority identity if it has not been created before + // If no name is specified on the command line, use "authority" + let identity_name = cmd.identity.clone().unwrap_or("authority".to_string()); + if opts.state.get_named_identity(&identity_name).await.is_err() { + opts.state.create_identity_with_name(&identity_name).await?; + }; + opts.state .create_node_with_optional_values(&cmd.node_name, &cmd.identity, &None) .await?; @@ -257,19 +264,13 @@ async fn start_authority_node( ) -> miette::Result<()> { let (opts, cmd) = args; - // Retrieve the authority identity if it has been created before - // otherwise create a new one - let identity_name = match &cmd.identity { - Some(identity_name) => opts.state.get_named_identity(identity_name).await?.name(), - None => match opts.state.get_default_named_identity().await { - Ok(identity) => identity.name(), - Err(_) => opts - .state - .create_identity_with_name("authority") - .await? - .name(), - }, + // Create the authority identity if it has not been created before + // If no name is specified on the command line, use "authority" + let identity_name = cmd.identity.clone().unwrap_or("authority".to_string()); + if opts.state.get_named_identity(&identity_name).await.is_err() { + opts.state.create_identity_with_name(&identity_name).await?; }; + let node = opts .state .create_node_with_optional_values(&cmd.node_name, &Some(identity_name), &None) @@ -323,7 +324,7 @@ fn parse_trusted_identities(values: &str) -> Result { #[cfg(test)] mod tests { - use ockam::identity::{identities, Identifier}; + use ockam::identity::{Identifier, identities}; use ockam_core::compat::collections::HashMap; use super::*; diff --git a/implementations/rust/ockam/ockam_command/tests/bats/authority.bats b/implementations/rust/ockam/ockam_command/tests/bats/authority.bats index 825181398ac..cccd7a7438b 100644 --- a/implementations/rust/ockam/ockam_command/tests/bats/authority.bats +++ b/implementations/rust/ockam/ockam_command/tests/bats/authority.bats @@ -24,6 +24,22 @@ teardown() { assert_output --partial "\"is_up\": true" } +@test "authority - an authority identity is created by default for the authority node" { + port="$(random_port)" + + trusted="{}" + run_success "$OCKAM" authority create --tcp-listener-address="127.0.0.1:$port" --project-identifier 1 --trusted-identities "$trusted" + run_success "$OCKAM" identity show authority +} + +@test "authority - an authority identity is created by default for the authority node - with a given name" { + port="$(random_port)" + + trusted="{}" + run_success "$OCKAM" authority create --tcp-listener-address="127.0.0.1:$port" --project-identifier 1 --trusted-identities "$trusted" --identity ockam + run_success "$OCKAM" identity show ockam +} + @test "authority - standalone authority, enrollers, members" { port="$(random_port)" From 1e46c7f994eac8c4329d59a2b44d59a075d1ad21 Mon Sep 17 00:00:00 2001 From: etorreborre Date: Wed, 29 Nov 2023 11:02:51 +0100 Subject: [PATCH 3/9] fix(rust): allow bats tests to run concurrently for nodes --- .../ockam/ockam_command/src/authority/create.rs | 10 +++++----- .../ockam/ockam_command/src/node/models/show.rs | 7 +++++++ .../rust/ockam/ockam_command/src/node/show.rs | 2 ++ .../rust/ockam/ockam_command/tests/bats/nodes.bats | 14 +++++++++++++- .../ockam_identity/src/models/utils/identifiers.rs | 14 +++++++------- 5 files changed, 34 insertions(+), 13 deletions(-) diff --git a/implementations/rust/ockam/ockam_command/src/authority/create.rs b/implementations/rust/ockam/ockam_command/src/authority/create.rs index f95ae94599c..80e7e818c74 100644 --- a/implementations/rust/ockam/ockam_command/src/authority/create.rs +++ b/implementations/rust/ockam/ockam_command/src/authority/create.rs @@ -4,11 +4,11 @@ use std::process; use clap::ArgGroup; use clap::Args; -use miette::{IntoDiagnostic, miette}; +use miette::{miette, IntoDiagnostic}; use serde::{Deserialize, Serialize}; -use ockam::Context; use ockam::identity::{AttributesEntry, Identifier}; +use ockam::Context; use ockam_api::authority_node; use ockam_api::authority_node::{OktaConfiguration, TrustedIdentity}; use ockam_api::bootstrapped_identities_store::PreTrustedIdentities; @@ -17,11 +17,11 @@ use ockam_api::nodes::service::default_address::DefaultAddress; use ockam_core::compat::collections::HashMap; use ockam_core::compat::fmt; -use crate::{CommandGlobalOpts, docs, Result}; use crate::node::util::run_ockam; +use crate::util::parsers::internet_address_parser; use crate::util::{embedded_node_that_is_not_stopped, exitcode}; use crate::util::{local_cmd, node_rpc}; -use crate::util::parsers::internet_address_parser; +use crate::{docs, CommandGlobalOpts, Result}; const LONG_ABOUT: &str = include_str!("./static/create/long_about.txt"); const PREVIEW_TAG: &str = include_str!("../static/preview_tag.txt"); @@ -324,7 +324,7 @@ fn parse_trusted_identities(values: &str) -> Result { #[cfg(test)] mod tests { - use ockam::identity::{Identifier, identities}; + use ockam::identity::{identities, Identifier}; use ockam_core::compat::collections::HashMap; use super::*; diff --git a/implementations/rust/ockam/ockam_command/src/node/models/show.rs b/implementations/rust/ockam/ockam_command/src/node/models/show.rs index 2cc857a3ce6..df74747d794 100644 --- a/implementations/rust/ockam/ockam_command/src/node/models/show.rs +++ b/implementations/rust/ockam/ockam_command/src/node/models/show.rs @@ -23,6 +23,7 @@ pub struct ShowNodeResponse { pub is_default: bool, pub name: String, pub is_up: bool, + pub node_pid: Option, pub route: RouteToNode, #[serde(skip_serializing_if = "Option::is_none")] pub identity: Option, @@ -46,6 +47,7 @@ impl ShowNodeResponse { name: &str, is_up: bool, node_port: Option, + node_pid: Option, ) -> ShowNodeResponse { let mut m = MultiAddr::default(); let short = m.push_back(Node::new(name)).ok().map(|_| m); @@ -64,6 +66,7 @@ impl ShowNodeResponse { is_default, name: name.to_owned(), is_up, + node_pid, route: RouteToNode { short, verbose }, identity: None, transports: Default::default(), @@ -94,6 +97,10 @@ impl Display for ShowNodeResponse { } )?; + if let Some(node_pid) = self.node_pid { + writeln!(buffer, " PID: {}", node_pid)?; + } + writeln!(buffer, " Route To Node:")?; if let Some(short) = &self.route.short { writeln!(buffer, " Short: {short}")?; diff --git a/implementations/rust/ockam/ockam_command/src/node/show.rs b/implementations/rust/ockam/ockam_command/src/node/show.rs index d9d1705cc9c..887ae5b1e0c 100644 --- a/implementations/rust/ockam/ockam_command/src/node/show.rs +++ b/implementations/rust/ockam/ockam_command/src/node/show.rs @@ -148,6 +148,7 @@ pub async fn print_query_status( &node_name, is_authority_node, node_info.tcp_listener_port(), + node_info.pid(), ) } else { let mut show_node = ShowNodeResponse::new( @@ -155,6 +156,7 @@ pub async fn print_query_status( &node_name, true, node_info.tcp_listener_port(), + node_info.pid(), ); // Get list of services for the node let services: ServiceList = node.ask(ctx, api::list_services()).await?; diff --git a/implementations/rust/ockam/ockam_command/tests/bats/nodes.bats b/implementations/rust/ockam/ockam_command/tests/bats/nodes.bats index 0b2edaa3a22..e02a3d13218 100644 --- a/implementations/rust/ockam/ockam_command/tests/bats/nodes.bats +++ b/implementations/rust/ockam/ockam_command/tests/bats/nodes.bats @@ -15,7 +15,19 @@ teardown() { # ===== UTILS force_kill_node() { - run killall ockam + max_retries=5 + i=0 + while [[ $i -lt $max_retries ]]; do + pid="$($OCKAM node show $1 --output json | jq .node_pid)" + run kill -9 $pid + # Killing a node created without `-f` leaves the + # process in a defunct state when running within Docker. + if ! ps -p $pid || ps -p $pid | grep defunct; then + return + fi + sleep 0.2 + ((i = i + 1)) + done } # ===== TESTS diff --git a/implementations/rust/ockam/ockam_identity/src/models/utils/identifiers.rs b/implementations/rust/ockam/ockam_identity/src/models/utils/identifiers.rs index a1a2219d8a7..e6944224852 100644 --- a/implementations/rust/ockam/ockam_identity/src/models/utils/identifiers.rs +++ b/implementations/rust/ockam/ockam_identity/src/models/utils/identifiers.rs @@ -3,17 +3,17 @@ use core::str::FromStr; use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; -use ockam_core::{Error, Result}; use ockam_core::compat::{string::String, vec::Vec}; use ockam_core::env::FromString; +use ockam_core::{Error, Result}; +use crate::models::{ChangeHash, CHANGE_HASH_LEN, IDENTIFIER_LEN}; use crate::{Identifier, IdentityError}; -use crate::models::{CHANGE_HASH_LEN, ChangeHash, IDENTIFIER_LEN}; impl Serialize for Identifier { fn serialize(&self, serializer: S) -> core::result::Result - where - S: Serializer, + where + S: Serializer, { serializer.serialize_str(&String::from(self)) } @@ -21,8 +21,8 @@ impl Serialize for Identifier { impl<'de> Deserialize<'de> for Identifier { fn deserialize(deserializer: D) -> core::result::Result - where - D: Deserializer<'de>, + where + D: Deserializer<'de>, { let str: String = Deserialize::deserialize(deserializer)?; @@ -198,7 +198,7 @@ impl AsRef<[u8]> for ChangeHash { #[cfg(test)] mod test { - use quickcheck::{Arbitrary, Gen, quickcheck}; + use quickcheck::{quickcheck, Arbitrary, Gen}; use super::*; From fa41cf34776d820633a15933465f82c5b64cbdb6 Mon Sep 17 00:00:00 2001 From: etorreborre Date: Wed, 29 Nov 2023 14:10:52 +0100 Subject: [PATCH 4/9] fix(rust): fix the creation of an identity with optional name and vault --- .../ockam_api/src/cli_state/identities.rs | 141 +++++++++++++++++- .../rust/ockam/ockam_identity/src/error.rs | 2 + 2 files changed, 137 insertions(+), 6 deletions(-) diff --git a/implementations/rust/ockam/ockam_api/src/cli_state/identities.rs b/implementations/rust/ockam/ockam_api/src/cli_state/identities.rs index b9085748a74..1fd36940d60 100644 --- a/implementations/rust/ockam/ockam_api/src/cli_state/identities.rs +++ b/implementations/rust/ockam/ockam_api/src/cli_state/identities.rs @@ -49,17 +49,40 @@ impl CliState { self.create_identity_with_name_and_vault(name, &vault.name()) .await } + /// Create an identity associated with an optional name and an optional vault name - /// If the vault name is not specified then the default vault is used + /// - if the vault name is not specified then the default vault is used + /// - if no name is specified: + /// - if there is no default identity, create an identity with a random name + /// - if there is a default identity with the correct vault, return it + /// - otherwise create a new identity + /// - if a name is given create a new identity with the proper vault (unless it has already been created) pub async fn create_identity_with_optional_name_and_optional_vault( &self, name: &Option, vault_name: &Option, ) -> Result { - let name = name.clone().unwrap_or_else(random_name); - let vault = self.get_named_vault_or_default(vault_name).await?.name(); - self.create_identity_with_name_and_vault(&name, &vault) - .await + let vault_name = self.get_named_vault_or_default(vault_name).await?.name(); + match name { + Some(name) => { + self.create_identity_with_name_and_vault(name, &vault_name) + .await + } + None => { + let default_identity = self + .identities_repository() + .await? + .get_default_named_identity() + .await?; + if let Some(default_identity) = default_identity { + if default_identity.vault_name == vault_name { + return Ok(default_identity); + } + }; + self.create_identity_with_name_and_vault(&random_name(), &vault_name) + .await + } + } } /// Create an identity with specific key id. @@ -332,7 +355,7 @@ impl CliState { node_names.join(", ") ), ) - .into()) + .into()) } } } @@ -509,6 +532,112 @@ mod tests { Ok(()) } + #[tokio::test] + async fn test_create_identity_with_a_name_and_no_vault() -> Result<()> { + let cli = CliState::test().await?; + + // create a named identity using the default vault + let identity = cli + .create_identity_with_optional_name_and_optional_vault(&Some("name".to_string()), &None) + .await?; + + // the created identity becomes the default one + let expected = cli.get_default_named_identity().await?; + assert_eq!(identity, expected); + + // the identity can be retrieved by name + let expected = cli.get_named_identity("name").await?; + assert_eq!(identity, expected); + + // the identity will not be recreated a second time + let identity = cli + .create_identity_with_optional_name_and_optional_vault(&Some("name".to_string()), &None) + .await?; + assert_eq!(identity, expected); + let identities = cli.get_named_identities().await?; + assert_eq!(identities.len(), 1); + + // the created identity uses the default vault + let default_vault = cli.get_default_named_vault().await?; + assert_eq!(identity.vault_name, default_vault.name()); + + Ok(()) + } + + #[tokio::test] + async fn test_create_identity_with_no_name_and_a_vault() -> Result<()> { + let cli = CliState::test().await?; + + // create a named identity using a given vault + let vault = cli.create_named_vault("vault").await?; + + let identity = cli + .create_identity_with_optional_name_and_optional_vault(&None, &Some(vault.name())) + .await?; + + // the created identity becomes the default one + let expected = cli.get_default_named_identity().await?; + assert_eq!(identity, expected); + + // the identity can be retrieved by name + let expected = cli.get_named_identity(&identity.name()).await?; + assert_eq!(identity, expected); + + // the identity will not be recreated a second time + let identity = cli + .create_identity_with_optional_name_and_optional_vault( + &Some(identity.name()), + &Some(vault.name()), + ) + .await?; + assert_eq!(identity, expected); + let identities = cli.get_named_identities().await?; + assert_eq!(identities.len(), 1); + + // the created identity uses the specified vault + assert_eq!(identity.vault_name, vault.name()); + + Ok(()) + } + + #[tokio::test] + async fn test_create_identity_with_no_name_and_a_non_default_vault() -> Result<()> { + let cli = CliState::test().await?; + + // create two vaults + let _ = cli.create_named_vault("vault1").await?; + let vault2 = cli.create_named_vault("vault2").await?; + + // create an identity with vault2 which is not the default vault + let identity = cli + .create_identity_with_optional_name_and_optional_vault(&None, &Some(vault2.name())) + .await?; + + // the created identity becomes the default one + let expected = cli.get_default_named_identity().await?; + assert_eq!(identity, expected); + + // the identity can be retrieved by name + let expected = cli.get_named_identity(&identity.name()).await?; + assert_eq!(identity, expected); + + // the identity will not be recreated a second time + let identity = cli + .create_identity_with_optional_name_and_optional_vault( + &Some(identity.name()), + &Some(vault2.name()), + ) + .await?; + assert_eq!(identity, expected); + let identities = cli.get_named_identities().await?; + assert_eq!(identities.len(), 1); + + // the created identity uses the specified vault + assert_eq!(identity.vault_name, vault2.name()); + + Ok(()) + } + #[tokio::test] async fn test_get_default_identity() -> Result<()> { let cli = CliState::test().await?; diff --git a/implementations/rust/ockam/ockam_identity/src/error.rs b/implementations/rust/ockam/ockam_identity/src/error.rs index 5973f0d9e5e..091e6738faf 100644 --- a/implementations/rust/ockam/ockam_identity/src/error.rs +++ b/implementations/rust/ockam/ockam_identity/src/error.rs @@ -1,3 +1,4 @@ +use ockam_core::compat::string::String; use ockam_core::{ errcode::{Kind, Origin}, Error, @@ -66,6 +67,7 @@ pub enum IdentityError { } impl ockam_core::compat::error::Error for IdentityError {} + impl core::fmt::Display for IdentityError { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { core::fmt::Debug::fmt(self, f) From e4303fcb2f7745994d6686f03348347464b5e891 Mon Sep 17 00:00:00 2001 From: etorreborre Date: Wed, 29 Nov 2023 15:19:02 +0100 Subject: [PATCH 5/9] refactor(rust): simplify by removing the possibility to create identities with default values --- .../ockam_api/src/cli_state/identities.rs | 163 +----------------- .../src/nodes/service/in_memory_node.rs | 19 +- .../ockam/ockam_command/src/message/send.rs | 3 +- 3 files changed, 16 insertions(+), 169 deletions(-) diff --git a/implementations/rust/ockam/ockam_api/src/cli_state/identities.rs b/implementations/rust/ockam/ockam_api/src/cli_state/identities.rs index 1fd36940d60..98655287d7f 100644 --- a/implementations/rust/ockam/ockam_api/src/cli_state/identities.rs +++ b/implementations/rust/ockam/ockam_api/src/cli_state/identities.rs @@ -50,41 +50,6 @@ impl CliState { .await } - /// Create an identity associated with an optional name and an optional vault name - /// - if the vault name is not specified then the default vault is used - /// - if no name is specified: - /// - if there is no default identity, create an identity with a random name - /// - if there is a default identity with the correct vault, return it - /// - otherwise create a new identity - /// - if a name is given create a new identity with the proper vault (unless it has already been created) - pub async fn create_identity_with_optional_name_and_optional_vault( - &self, - name: &Option, - vault_name: &Option, - ) -> Result { - let vault_name = self.get_named_vault_or_default(vault_name).await?.name(); - match name { - Some(name) => { - self.create_identity_with_name_and_vault(name, &vault_name) - .await - } - None => { - let default_identity = self - .identities_repository() - .await? - .get_default_named_identity() - .await?; - if let Some(default_identity) = default_identity { - if default_identity.vault_name == vault_name { - return Ok(default_identity); - } - }; - self.create_identity_with_name_and_vault(&random_name(), &vault_name) - .await - } - } - } - /// Create an identity with specific key id. /// This method is used when the vault is a KMS vault and we just need to store a key id /// for the identity key existing in the KMS @@ -514,130 +479,6 @@ mod tests { Ok(()) } - #[tokio::test] - async fn test_create_default_identity() -> Result<()> { - let cli = CliState::test().await?; - - // create a default identity using the default vault - let identity = cli - .create_identity_with_optional_name_and_optional_vault(&None, &None) - .await?; - let expected = cli.get_default_named_identity().await?; - assert_eq!(identity, expected); - - // check that the identity uses the default vault - let default_vault = cli.get_default_named_vault().await?; - assert_eq!(identity.vault_name, default_vault.name()); - - Ok(()) - } - - #[tokio::test] - async fn test_create_identity_with_a_name_and_no_vault() -> Result<()> { - let cli = CliState::test().await?; - - // create a named identity using the default vault - let identity = cli - .create_identity_with_optional_name_and_optional_vault(&Some("name".to_string()), &None) - .await?; - - // the created identity becomes the default one - let expected = cli.get_default_named_identity().await?; - assert_eq!(identity, expected); - - // the identity can be retrieved by name - let expected = cli.get_named_identity("name").await?; - assert_eq!(identity, expected); - - // the identity will not be recreated a second time - let identity = cli - .create_identity_with_optional_name_and_optional_vault(&Some("name".to_string()), &None) - .await?; - assert_eq!(identity, expected); - let identities = cli.get_named_identities().await?; - assert_eq!(identities.len(), 1); - - // the created identity uses the default vault - let default_vault = cli.get_default_named_vault().await?; - assert_eq!(identity.vault_name, default_vault.name()); - - Ok(()) - } - - #[tokio::test] - async fn test_create_identity_with_no_name_and_a_vault() -> Result<()> { - let cli = CliState::test().await?; - - // create a named identity using a given vault - let vault = cli.create_named_vault("vault").await?; - - let identity = cli - .create_identity_with_optional_name_and_optional_vault(&None, &Some(vault.name())) - .await?; - - // the created identity becomes the default one - let expected = cli.get_default_named_identity().await?; - assert_eq!(identity, expected); - - // the identity can be retrieved by name - let expected = cli.get_named_identity(&identity.name()).await?; - assert_eq!(identity, expected); - - // the identity will not be recreated a second time - let identity = cli - .create_identity_with_optional_name_and_optional_vault( - &Some(identity.name()), - &Some(vault.name()), - ) - .await?; - assert_eq!(identity, expected); - let identities = cli.get_named_identities().await?; - assert_eq!(identities.len(), 1); - - // the created identity uses the specified vault - assert_eq!(identity.vault_name, vault.name()); - - Ok(()) - } - - #[tokio::test] - async fn test_create_identity_with_no_name_and_a_non_default_vault() -> Result<()> { - let cli = CliState::test().await?; - - // create two vaults - let _ = cli.create_named_vault("vault1").await?; - let vault2 = cli.create_named_vault("vault2").await?; - - // create an identity with vault2 which is not the default vault - let identity = cli - .create_identity_with_optional_name_and_optional_vault(&None, &Some(vault2.name())) - .await?; - - // the created identity becomes the default one - let expected = cli.get_default_named_identity().await?; - assert_eq!(identity, expected); - - // the identity can be retrieved by name - let expected = cli.get_named_identity(&identity.name()).await?; - assert_eq!(identity, expected); - - // the identity will not be recreated a second time - let identity = cli - .create_identity_with_optional_name_and_optional_vault( - &Some(identity.name()), - &Some(vault2.name()), - ) - .await?; - assert_eq!(identity, expected); - let identities = cli.get_named_identities().await?; - assert_eq!(identities.len(), 1); - - // the created identity uses the specified vault - assert_eq!(identity.vault_name, vault2.name()); - - Ok(()) - } - #[tokio::test] async fn test_get_default_identity() -> Result<()> { let cli = CliState::test().await?; @@ -652,6 +493,10 @@ mod tests { let result = cli.get_named_identity(&identity.name()).await; assert!(result.is_ok()); + // check that the identity uses the default vault + let default_vault = cli.get_default_named_vault().await?; + assert_eq!(identity.vault_name, default_vault.name()); + Ok(()) } diff --git a/implementations/rust/ockam/ockam_api/src/nodes/service/in_memory_node.rs b/implementations/rust/ockam/ockam_api/src/nodes/service/in_memory_node.rs index c1407178cd3..a3f09ac33b1 100644 --- a/implementations/rust/ockam/ockam_api/src/nodes/service/in_memory_node.rs +++ b/implementations/rust/ockam/ockam_api/src/nodes/service/in_memory_node.rs @@ -76,28 +76,31 @@ impl InMemoryNode { project_name: Option, trust_context: Option, ) -> miette::Result { - Self::start_node(ctx, cli_state, None, None, project_name, trust_context).await + let default_identity_name = cli_state.get_default_named_identity().await?.name(); + Self::start_node( + ctx, + cli_state, + &default_identity_name, + project_name, + trust_context, + ) + .await } /// Start an in memory node pub async fn start_node( ctx: &Context, cli_state: &CliState, - identity_name: Option, - vault_name: Option, + identity_name: &str, project_name: Option, trust_context: Option, ) -> miette::Result { let defaults = NodeManagerDefaults::default(); - // if no identity is specified, create one - let identity = cli_state - .create_identity_with_optional_name_and_optional_vault(&identity_name, &vault_name) - .await?; let node = cli_state .create_node_with_optional_values( &defaults.node_name, - &Some(identity.name()), + &Some(identity_name.to_string()), &project_name, ) .await?; diff --git a/implementations/rust/ockam/ockam_command/src/message/send.rs b/implementations/rust/ockam/ockam_command/src/message/send.rs index 670d9cfa022..7a56a5908e5 100644 --- a/implementations/rust/ockam/ockam_command/src/message/send.rs +++ b/implementations/rust/ockam/ockam_command/src/message/send.rs @@ -109,8 +109,7 @@ async fn rpc(ctx: Context, (opts, cmd): (CommandGlobalOpts, SendCommand)) -> mie let node_manager = InMemoryNode::start_node( ctx, &opts.state, - Some(identity_name.clone()), - None, + &identity_name, cmd.trust_context_opts.project_name, named_trust_context, ) From f2a44fb2330e6747965fffcc23f4889f38ea7b2a Mon Sep 17 00:00:00 2001 From: etorreborre Date: Wed, 29 Nov 2023 16:31:59 +0100 Subject: [PATCH 6/9] style(rust): fix the bash formatting for bats support functions --- .../ockam/ockam_command/tests/bats/nodes.bats | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/implementations/rust/ockam/ockam_command/tests/bats/nodes.bats b/implementations/rust/ockam/ockam_command/tests/bats/nodes.bats index e02a3d13218..5eb5b863152 100644 --- a/implementations/rust/ockam/ockam_command/tests/bats/nodes.bats +++ b/implementations/rust/ockam/ockam_command/tests/bats/nodes.bats @@ -16,18 +16,18 @@ teardown() { force_kill_node() { max_retries=5 - i=0 - while [[ $i -lt $max_retries ]]; do - pid="$($OCKAM node show $1 --output json | jq .node_pid)" - run kill -9 $pid - # Killing a node created without `-f` leaves the - # process in a defunct state when running within Docker. - if ! ps -p $pid || ps -p $pid | grep defunct; then - return - fi - sleep 0.2 - ((i = i + 1)) - done + i=0 + while [[ $i -lt $max_retries ]]; do + pid="$($OCKAM node show $1 --output json | jq .node_pid)" + run kill -9 $pid + # Killing a node created without `-f` leaves the + # process in a defunct state when running within Docker. + if ! ps -p $pid || ps -p $pid | grep defunct; then + return + fi + sleep 0.2 + ((i = i + 1)) + done } # ===== TESTS From 3d2d4bcf4889424c8f440e1dae995cdff3901dc0 Mon Sep 17 00:00:00 2001 From: etorreborre Date: Wed, 29 Nov 2023 16:39:14 +0100 Subject: [PATCH 7/9] fix(rust): allow a project to be stored with any authority informtion --- .../ockam/ockam_api/src/cli_state/projects.rs | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/implementations/rust/ockam/ockam_api/src/cli_state/projects.rs b/implementations/rust/ockam/ockam_api/src/cli_state/projects.rs index 75e0ffebddb..04407954582 100644 --- a/implementations/rust/ockam/ockam_api/src/cli_state/projects.rs +++ b/implementations/rust/ockam/ockam_api/src/cli_state/projects.rs @@ -59,8 +59,8 @@ impl CliState { Some(project.name()), Some(project.id()), None, - Some(project.authority_identity().await?), - Some(project.authority_access_route()?), + project.authority_identity().await.ok(), + project.authority_access_route().ok(), ) .await?; Ok(()) @@ -152,3 +152,27 @@ impl CliState { Ok(projects) } } + +#[cfg(test)] +mod tests { + use ockam_core::env::FromString; + + use super::*; + + #[tokio::test] + async fn test_import_project() -> Result<()> { + let cli = CliState::test().await?; + + // a project can be created without specifying its authority + cli.import_project( + "project_id", + "project_name", + &None, + &MultiAddr::from_string("/project/default").unwrap(), + &None, + &None, + ) + .await?; + Ok(()) + } +} From 3860869b25d05ec252e7c5045f9605c8b68dc25e Mon Sep 17 00:00:00 2001 From: etorreborre Date: Wed, 29 Nov 2023 17:27:02 +0100 Subject: [PATCH 8/9] fix(rust): update the project state when it is ready --- .../rust/ockam/ockam_command/src/enroll/command.rs | 4 +++- .../rust/ockam/ockam_command/src/project/create.rs | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/implementations/rust/ockam/ockam_command/src/enroll/command.rs b/implementations/rust/ockam/ockam_command/src/enroll/command.rs index cd53f5ee32c..8b728387cbc 100644 --- a/implementations/rust/ockam/ockam_command/src/enroll/command.rs +++ b/implementations/rust/ockam/ockam_command/src/enroll/command.rs @@ -346,7 +346,9 @@ async fn get_user_project( } }; - check_project_readiness(opts, ctx, node, project.clone()).await?; + let project = check_project_readiness(opts, ctx, node, project.clone()).await?; + // store the updated project + opts.state.store_project(project.clone()).await?; opts.terminal.write_line(&fmt_ok!( "Marked this project as your default project, on this machine.\n" diff --git a/implementations/rust/ockam/ockam_command/src/project/create.rs b/implementations/rust/ockam/ockam_command/src/project/create.rs index e180256b4a0..4330d793ca2 100644 --- a/implementations/rust/ockam/ockam_command/src/project/create.rs +++ b/implementations/rust/ockam/ockam_command/src/project/create.rs @@ -58,6 +58,8 @@ async fn run_impl( let controller = node.create_controller().await?; check_for_completion(&opts, ctx, &controller, &operation_id).await?; let project = check_project_readiness(&opts, ctx, &node, project).await?; + // update the project state when it's ready + opts.state.store_project(project.clone()).await?; opts.println(&project)?; Ok(()) } From 3d2888de884c488006b8efb507363bf5a0b87dce Mon Sep 17 00:00:00 2001 From: etorreborre Date: Wed, 29 Nov 2023 18:52:20 +0100 Subject: [PATCH 9/9] test(rust): remove the reset on teardown for the bats tests --- .../rust/ockam/ockam_command/tests/bats/load/base.bash | 1 - 1 file changed, 1 deletion(-) diff --git a/implementations/rust/ockam/ockam_command/tests/bats/load/base.bash b/implementations/rust/ockam/ockam_command/tests/bats/load/base.bash index c8ce8d06af2..33b0cfb646d 100644 --- a/implementations/rust/ockam/ockam_command/tests/bats/load/base.bash +++ b/implementations/rust/ockam/ockam_command/tests/bats/load/base.bash @@ -87,7 +87,6 @@ teardown_home_dir() { cp -r "$OCKAM_HOME/." "$HOME/.bats-tests" fi run $OCKAM node delete --all --force --yes - run $OCKAM reset -y done }