Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(connector): Use ConnectorError::InvalidConnectorConfig for an invalid CoinbaseConnectorMeta #3168

Merged
merged 18 commits into from
Jan 25, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions crates/router/src/connector/coinbase/transformers.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use std::collections::HashMap;

use common_utils::pii;
use error_stack::ResultExt;
use serde::{Deserialize, Serialize};

use crate::{
Expand Down Expand Up @@ -250,6 +252,14 @@ pub struct CoinbaseConnectorMeta {
pub pricing_type: String,
}

impl TryFrom<&Option<pii::SecretSerdeValue>> for CoinbaseConnectorMeta {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes will impact previous merchants who are using coinbase, This PR needs data migrations. Could you please refer #2746 description for more info.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue#2899 does not mention any data migrations.
I've checked that #2746, and in that case, the migration involved replacing terminalId with terminal_id due to the removal of #[serde(rename_all = "camelCase")] here.

However, my issue is different. If I understand correctly, this issue is about empty JSON for CoinbaseConnectorMeta or the absence of the pricing_type attribute in the JSON, and the goal is to add validations to prevent storing invalid CoinbaseConnectorMeta.

So, I am not sure about the migration here. If empty JSON or JSON without pricing_type is stored, how should the migration to the correct CoinbaseConnectorMeta be handled? What value should be set for pricing_type?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, We will take this up as these are data migrations. FYI, I was talking about existing merchants, we need to add pricing_type in metadata and set it to empty string. In that case, when merchant tries to make payment connector throws error and they are expected to update the metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srujanchikke aha, ok, thank you, make senses now, so I prepared migration script and simple test demonstration that it works, please see PR description.

type Error = error_stack::Report<errors::ConnectorError>;
fn try_from(meta_data: &Option<pii::SecretSerdeValue>) -> Result<Self, Self::Error> {
utils::to_connector_meta_from_secret(meta_data.clone())
.change_context(errors::ConnectorError::InvalidConnectorConfig { config: "metadata" })
}
}

fn get_crypto_specific_payment_data(
item: &types::PaymentsAuthorizeRouterData,
) -> Result<CoinbasePaymentsRequest, error_stack::Report<errors::ConnectorError>> {
Expand All @@ -260,11 +270,7 @@ fn get_crypto_specific_payment_data(
let name =
billing_address.and_then(|add| add.get_first_name().ok().map(|name| name.to_owned()));
let description = item.get_description().ok();
let connector_meta: CoinbaseConnectorMeta =
utils::to_connector_meta_from_secret_with_required_field(
item.connector_meta_data.clone(),
"Pricing Type Not present in connector meta data",
)?;
let connector_meta = CoinbaseConnectorMeta::try_from(&item.connector_meta_data)?;
bkontur marked this conversation as resolved.
Show resolved Hide resolved
let pricing_type = connector_meta.pricing_type;
let local_price = get_local_price(item);
let redirect_url = item.request.get_return_url()?;
Expand Down
17 changes: 0 additions & 17 deletions crates/router/src/connector/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1198,23 +1198,6 @@ where
json.parse_value(std::any::type_name::<T>()).switch()
}

pub fn to_connector_meta_from_secret_with_required_field<T>(
connector_meta: Option<Secret<serde_json::Value>>,
error_message: &'static str,
) -> Result<T, Error>
where
T: serde::de::DeserializeOwned,
{
let connector_error = errors::ConnectorError::MissingRequiredField {
field_name: error_message,
};
let parsed_meta = to_connector_meta_from_secret(connector_meta).ok();
match parsed_meta {
Some(meta) => Ok(meta),
_ => Err(connector_error.into()),
}
}

pub fn to_connector_meta_from_secret<T>(
connector_meta: Option<Secret<serde_json::Value>>,
) -> Result<T, Error>
Expand Down
2 changes: 1 addition & 1 deletion crates/router/src/core/admin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1685,9 +1685,9 @@ pub(crate) fn validate_auth_and_metadata_type(
checkout::transformers::CheckoutAuthType::try_from(val)?;
Ok(())
}

api_enums::Connector::Coinbase => {
coinbase::transformers::CoinbaseAuthType::try_from(val)?;
let _ = coinbase::transformers::CoinbaseConnectorMeta::try_from(connector_meta_data)?;
bkontur marked this conversation as resolved.
Show resolved Hide resolved
Ok(())
}
api_enums::Connector::Cryptopay => {
Expand Down
115 changes: 110 additions & 5 deletions crates/router/tests/connectors/coinbase.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
use std::marker::PhantomData;
bkontur marked this conversation as resolved.
Show resolved Hide resolved

use api_models::payments::CryptoData;
use common_utils::pii;
use masking::Secret;
use router::types::{self, api, storage::enums, PaymentAddress};
use router::{
connector::coinbase::transformers::{CoinbasePaymentsRequest, LocalPrice},
core::errors::ConnectorError,
types::{self, api, storage::enums, PaymentAddress},
};
use serde_json::json;

use crate::{
Expand Down Expand Up @@ -64,8 +71,8 @@ fn get_default_payment_info() -> Option<utils::PaymentInfo> {
})
}

fn payment_method_details() -> Option<types::PaymentsAuthorizeData> {
Some(types::PaymentsAuthorizeData {
fn payment_method_details() -> types::PaymentsAuthorizeData {
types::PaymentsAuthorizeData {
amount: 1,
currency: enums::Currency::USD,
payment_method_data: types::api::PaymentMethodData::Crypto(CryptoData {
Expand Down Expand Up @@ -96,14 +103,62 @@ fn payment_method_details() -> Option<types::PaymentsAuthorizeData> {
surcharge_details: None,
request_incremental_authorization: false,
metadata: None,
})
}
}

fn construct_payment_router_data(
connector_meta_data: Option<pii::SecretSerdeValue>,
) -> types::PaymentsAuthorizeRouterData {
let connector_auth_type = types::ConnectorAuthType::HeaderKey {
api_key: Secret::new("api_key".to_string()),
};

types::RouterData {
flow: PhantomData,
merchant_id: String::from("Coinbase"),
customer_id: Some(String::from("Coinbase")),
connector: "Coinbase".to_string(),
payment_id: uuid::Uuid::new_v4().to_string(),
attempt_id: uuid::Uuid::new_v4().to_string(),
status: Default::default(),
auth_type: enums::AuthenticationType::NoThreeDs,
payment_method: enums::PaymentMethod::Card,
connector_auth_type,
description: Some("This is a test".to_string()),
return_url: None,
request: payment_method_details(),
response: Err(Default::default()),
payment_method_id: None,
address: Default::default(),
connector_meta_data,
amount_captured: None,
access_token: None,
session_token: None,
reference_id: None,
payment_method_token: None,
connector_customer: None,
recurring_mandate_payment_data: None,
preprocessing_id: None,
connector_request_reference_id: uuid::Uuid::new_v4().to_string(),
#[cfg(feature = "payouts")]
payout_method_data: None,
#[cfg(feature = "payouts")]
quote_id: None,
test_mode: None,
payment_method_balance: None,
connector_api_version: None,
connector_http_status_code: None,
apple_pay_flow: None,
external_latency: None,
frm_metadata: None,
}
}

// Creates a payment using the manual capture flow
#[actix_web::test]
async fn should_only_authorize_payment() {
let response = CONNECTOR
.authorize_payment(payment_method_details(), get_default_payment_info())
.authorize_payment(Some(payment_method_details()), get_default_payment_info())
.await
.expect("Authorize payment response");
assert_eq!(response.status, enums::AttemptStatus::AuthenticationPending);
Expand Down Expand Up @@ -192,3 +247,53 @@ async fn should_sync_cancelled_payment() {
.expect("PSync response");
assert_eq!(response.status, enums::AttemptStatus::Voided);
}

#[test]
fn coinbase_payments_request_try_from_works() {
// `connector_meta_data` as `None` - should fail
assert_eq!(
CoinbasePaymentsRequest::try_from(&construct_payment_router_data(None))
.unwrap_err()
.current_context(),
&ConnectorError::InvalidConnectorConfig { config: "metadata" },
);

// `connector_meta_data` as empty json - should fail
assert_eq!(
CoinbasePaymentsRequest::try_from(&construct_payment_router_data(Some(Secret::new(
serde_json::json!({})
))))
.unwrap_err()
.current_context(),
&ConnectorError::InvalidConnectorConfig { config: "metadata" },
);

// `connector_meta_data` as json with missing `pricing_type` - should fail
assert_eq!(
CoinbasePaymentsRequest::try_from(&construct_payment_router_data(Some(Secret::new(
serde_json::json!({ "wrong_type" : "blah" })
))))
.unwrap_err()
.current_context(),
&ConnectorError::InvalidConnectorConfig { config: "metadata" },
);

// `connector_meta_data` as json with correct `pricing_type` - ok
assert_eq!(
CoinbasePaymentsRequest::try_from(&construct_payment_router_data(Some(Secret::new(
serde_json::json!({ "pricing_type" : "fixed_price" })
))))
.unwrap(),
CoinbasePaymentsRequest {
name: None,
description: Some("This is a test".to_string()),
pricing_type: "fixed_price".to_string(),
local_price: LocalPrice {
amount: "1".to_string(),
currency: "USD".to_string()
},
redirect_url: "https://google.com/".to_string(),
cancel_url: "https://google.com/".to_string(),
}
);
}
Loading