diff --git a/integration-tests/tests/lib.rs b/integration-tests/tests/lib.rs index f7570dbc7..a5e1a5fd8 100644 --- a/integration-tests/tests/lib.rs +++ b/integration-tests/tests/lib.rs @@ -217,6 +217,8 @@ trait MpcCheck { fn assert_ok(self) -> anyhow::Result; fn assert_bad_request_contains(self, expected: &str) -> anyhow::Result; fn assert_unauthorized_contains(self, expected: &str) -> anyhow::Result; + fn assert_internal_error_contains(self, expected: &str) -> anyhow::Result; + fn assert_dependency_error_contains(self, expected: &str) -> anyhow::Result; fn assert_bad_request(self) -> anyhow::Result where @@ -230,6 +232,12 @@ trait MpcCheck { { self.assert_unauthorized_contains("") } + fn assert_internal_error(self) -> anyhow::Result + where + Self: Sized, + { + self.assert_internal_error_contains("") + } } // Presumes that $response::Err has a `msg: String` field. @@ -297,6 +305,47 @@ macro_rules! impl_mpc_check { ); } } + // ideally we should not have situations where we can get INTERNAL_SERVER_ERROR + fn assert_internal_error_contains( + self, + expected: &str, + ) -> anyhow::Result { + let status_code = self.0; + let response = self.1; + + if status_code == StatusCode::INTERNAL_SERVER_ERROR { + let $response::Err { ref msg, .. } = response else { + anyhow::bail!("unexpected error with a 401 http code"); + }; + assert!(msg.contains(expected)); + + Ok(response) + } else { + anyhow::bail!( + "expected 401, but got {status_code} with response: {response:?}" + ); + } + } + fn assert_dependency_error_contains( + self, + expected: &str, + ) -> anyhow::Result { + let status_code = self.0; + let response = self.1; + + if status_code == StatusCode::FAILED_DEPENDENCY { + let $response::Err { ref msg, .. } = response else { + anyhow::bail!("unexpected error with a 424 http code"); + }; + assert!(msg.contains(expected)); + + Ok(response) + } else { + anyhow::bail!( + "expected 424, but got {status_code} with response: {response:?}" + ); + } + } } }; } diff --git a/integration-tests/tests/mpc/negative.rs b/integration-tests/tests/mpc/negative.rs index 7b08ce467..b3323a293 100644 --- a/integration-tests/tests/mpc/negative.rs +++ b/integration-tests/tests/mpc/negative.rs @@ -206,18 +206,16 @@ async fn negative_front_running_protection() -> anyhow::Result<()> { let wrong_oidc_token = OidcToken::random(); // Create account before claiming OIDC token - // This part of the test is commented since account creation is not atomic (known issue) - // Relayer is wasting a token even if account was not created - // ctx.leader_node - // .new_account_with_helper( - // account_id.to_string(), - // user_public_key.clone(), - // None, - // user_secret_key.clone(), - // oidc_token_1.clone(), - // ) - // .await? - // .assert_unauthorized_contains("was not claimed")?; + ctx.leader_node + .new_account_with_helper( + &account_id, + &user_public_key, + None, + &user_secret_key, + &oidc_token_1, + ) + .await? + .assert_unauthorized_contains("was not claimed")?; // Get user recovery PK before claiming OIDC token ctx.leader_node @@ -605,3 +603,85 @@ async fn test_malformed_raw_create_account() -> anyhow::Result<()> { }) .await } + +#[test(tokio::test)] +async fn test_account_creation_should_work_on_second_attempt() -> anyhow::Result<()> { + with_nodes(2, |ctx| { + Box::pin(async move { + let account_id = account::random(ctx.worker)?; + let user_secret_key = key::random_sk(); + let user_public_key = user_secret_key.public_key(); + let oidc_token = OidcToken::random(); + + ctx.leader_node + .new_account_with_helper( + &account_id, + &user_public_key, + None, + &user_secret_key, + &oidc_token, + ) + .await? + .assert_unauthorized_contains("was not claimed")?; + + register_account( + &ctx, + &account_id, + &user_secret_key, + &user_public_key, + &oidc_token, + None, + ) + .await?; + + Ok(()) + }) + }) + .await +} + +#[test(tokio::test)] +async fn test_creation_of_two_account_with_the_same_oidc_should_not_be_possible( +) -> anyhow::Result<()> { + with_nodes(2, |ctx| { + Box::pin(async move { + let account_id = account::random(ctx.worker)?; + let account_id_2 = account::random(ctx.worker)?; + let user_secret_key = key::random_sk(); + let user_public_key = user_secret_key.public_key(); + let oidc_token = OidcToken::random(); + + ctx.leader_node + .claim_oidc_with_helper(&oidc_token, &user_public_key, &user_secret_key) + .await? + .assert_ok()?; + + ctx.leader_node + .new_account_with_helper( + &account_id, + &user_public_key, + None, + &user_secret_key, + &oidc_token, + ) + .await? + .assert_ok()?; + + ctx.leader_node + .new_account_with_helper( + &account_id_2, + &user_public_key, + None, + &user_secret_key, + &oidc_token, + ) + .await? + .assert_dependency_error_contains( + "You can only register 1 account per oauth_token", + )?; + + Ok(()) + }) + }) + .await +} diff --git a/mpc-recovery/src/leader_node/mod.rs b/mpc-recovery/src/leader_node/mod.rs index 3edb0b6c9..2d484b04c 100644 --- a/mpc-recovery/src/leader_node/mod.rs +++ b/mpc-recovery/src/leader_node/mod.rs @@ -7,7 +7,7 @@ use crate::msg::{ SignRequest, SignResponse, UserCredentialsRequest, UserCredentialsResponse, }; use crate::oauth::OAuthTokenVerifier; -use crate::relayer::msg::RegisterAccountRequest; +use crate::relayer::msg::{CreateAccountAtomicRequest, RegisterAccountRequest}; use crate::relayer::NearRpcAndRelayerClient; use crate::transaction::{ get_create_account_delegate_action, get_local_signed_delegated_action, get_mpc_signature, @@ -32,7 +32,6 @@ use near_crypto::SecretKey; use near_primitives::delegate_action::{DelegateAction, NonDelegateAction}; use near_primitives::transaction::{Action, DeleteKeyAction}; use near_primitives::types::AccountId; -use near_primitives::views::FinalExecutionStatus; use prometheus::{Encoder, TextEncoder}; use rand::{distributions::Alphanumeric, Rng}; use std::net::SocketAddr; @@ -356,22 +355,6 @@ async fn process_new_account( .map_err(LeaderNodeError::SignatureVerificationFailed)?; tracing::debug!("user credentials digest signature verified for {new_user_account_id:?}"); - let partner = state - .partners - .find(&oidc_token_claims.iss, &oidc_token_claims.aud)?; - - state - .client - .register_account( - RegisterAccountRequest { - account_id: new_user_account_id.clone(), - allowance: 300_000_000_000_000, - oauth_token: internal_acc_id.clone(), - }, - partner.relayer, - ) - .await?; - nar::retry(|| async { // Get nonce and recent block hash let (_hash, block_height, nonce) = state @@ -417,40 +400,50 @@ async fn process_new_account( ); // Send delegate action to relayer + let request = CreateAccountAtomicRequest { + account_id: new_user_account_id.clone(), + allowance: 300_000_000_000_000, + oauth_token: internal_acc_id.clone(), + signed_delegate_action, + }; + + // TODO: move error message from here to this place let partner = state .partners .find(&oidc_token_claims.iss, &oidc_token_claims.aud)?; + let result = state .client - .send_meta_tx(signed_delegate_action, partner.relayer) + .create_account_atomic(request, partner.relayer) .await; - if let Err(err) = &result { - let err_str = format!("{:?}", err); - state - .client - .invalidate_cache_if_tx_failed( - &( - state.account_creator_id.clone(), - state.account_creator_sk.public_key(), - ), - &err_str, - ) - .await; - } - let response = result?; - - // TODO: Probably need to check more fields - if matches!(response.status, FinalExecutionStatus::SuccessValue(_)) { - Ok(NewAccountResponse::Ok { - create_account_options: new_account_options, - user_recovery_public_key: mpc_user_recovery_pk, - near_account_id: new_user_account_id.clone(), - }) - } else { - Err(LeaderNodeError::Other(anyhow::anyhow!( - "transaction failed with {:?}", - response.status - ))) + + match result { + Ok(_) => { + tracing::info!( + "account creation succeeded: {new_user_account_id:?}", + new_user_account_id = new_user_account_id + ); + Ok(NewAccountResponse::Ok { + create_account_options: new_account_options, + user_recovery_public_key: mpc_user_recovery_pk, + near_account_id: new_user_account_id.clone(), + }) + } + Err(err) => { + tracing::error!("account creation failed: {err}"); + let err_str = format!("{:?}", err); + state + .client + .invalidate_cache_if_acc_creation_failed( + &( + state.account_creator_id.clone(), + state.account_creator_sk.public_key(), + ), + &err_str, + ) + .await; + Err(LeaderNodeError::RelayerError(err)) + } } }) .await diff --git a/mpc-recovery/src/relayer/mod.rs b/mpc-recovery/src/relayer/mod.rs index a7cdabf5a..5b2c0f696 100644 --- a/mpc-recovery/src/relayer/mod.rs +++ b/mpc-recovery/src/relayer/mod.rs @@ -10,7 +10,9 @@ use near_primitives::types::{AccountId, BlockHeight, Nonce}; use near_primitives::views::FinalExecutionStatus; use self::error::RelayerError; -use self::msg::{RegisterAccountRequest, SendMetaTxRequest, SendMetaTxResponse}; +use self::msg::{ + CreateAccountAtomicRequest, RegisterAccountRequest, SendMetaTxRequest, SendMetaTxResponse, +}; use crate::firewall::allowed::DelegateActionRelayer; use crate::nar::{self, CachedAccessKeyNonces}; @@ -95,6 +97,50 @@ impl NearRpcAndRelayerClient { } } + #[tracing::instrument(level = "debug", skip_all, fields(account_id = request.account_id.to_string()))] + pub async fn create_account_atomic( + &self, + request: CreateAccountAtomicRequest, + relayer: DelegateActionRelayer, + ) -> Result<(), RelayerError> { + let mut req = Request::builder() + .method(Method::POST) + .uri(format!("{}/create_account_atomic", relayer.url)) + .header("content-type", "application/json"); + + if let Some(api_key) = relayer.api_key { + req = req.header("x-api-key", api_key); + }; + + let request = req + .body(Body::from( + serde_json::to_vec(&request) + .map_err(|e| RelayerError::DataConversionFailure(e.into()))?, + )) + .map_err(|e| RelayerError::NetworkFailure(e.into()))?; + + tracing::debug!("constructed http request to {}", relayer.url); + let client = Client::new(); + let response = client + .request(request) + .await + .map_err(|e| RelayerError::NetworkFailure(e.into()))?; + + let status = response.status(); + let response_body = hyper::body::to_bytes(response.into_body()) + .await + .map_err(|e| RelayerError::NetworkFailure(e.into()))?; + let msg = std::str::from_utf8(&response_body) + .map_err(|e| RelayerError::DataConversionFailure(e.into()))?; + + if status.is_success() { + tracing::debug!(response_body = msg, "got response"); + Ok(()) + } else { + Err(RelayerError::RequestFailure(status, msg.to_string())) + } + } + #[tracing::instrument(level = "debug", skip_all, fields(receiver_id = request.delegate_action.receiver_id.to_string()))] pub async fn send_meta_tx( &self, @@ -149,7 +195,7 @@ impl NearRpcAndRelayerClient { } } - pub(crate) async fn invalidate_cache_if_tx_failed( + pub(crate) async fn invalidate_cache_if_acc_creation_failed( &self, cache_key: &(AccountId, PublicKey), err_str: &str, diff --git a/mpc-recovery/src/relayer/msg.rs b/mpc-recovery/src/relayer/msg.rs index 9ce77d0a2..9357814cd 100644 --- a/mpc-recovery/src/relayer/msg.rs +++ b/mpc-recovery/src/relayer/msg.rs @@ -14,6 +14,16 @@ pub struct RegisterAccountRequest { pub oauth_token: String, } +#[derive(Clone, Debug, Serialize, Deserialize)] +pub struct CreateAccountAtomicRequest { + pub account_id: AccountId, + pub allowance: u64, + // This is actually an InternalAccountId. + // TODO: rename it to internal_account_id on the relayer side + pub oauth_token: String, + pub signed_delegate_action: SignedDelegateAction, +} + pub type SendMetaTxRequest = SignedDelegateAction; #[derive(Clone, Debug, Serialize, Deserialize)]