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 7 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
2 changes: 1 addition & 1 deletion crates/router/src/configs/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,7 @@ impl From<Database> for storage_impl::config::Database {
dbname: val.dbname,
pool_size: val.pool_size,
connection_timeout: val.connection_timeout,
queue_strategy: val.queue_strategy.into(),
queue_strategy: val.queue_strategy,
bkontur marked this conversation as resolved.
Show resolved Hide resolved
min_idle: val.min_idle,
max_lifetime: val.max_lifetime,
}
Expand Down
175 changes: 170 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,17 @@ 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: "`pricing_type` not present in `CoinbaseConnectorMeta`",
bkontur marked this conversation as resolved.
Show resolved Hide resolved
},
)
}
}

fn get_crypto_specific_payment_data(
item: &types::PaymentsAuthorizeRouterData,
) -> Result<CoinbasePaymentsRequest, error_stack::Report<errors::ConnectorError>> {
Expand All @@ -260,11 +273,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 Expand Up @@ -419,3 +428,159 @@ pub struct TimelinePayment {
pub network: String,
pub transaction_id: String,
}

#[cfg(test)]
mod tests {
bkontur marked this conversation as resolved.
Show resolved Hide resolved
#![allow(clippy::unwrap_used)]

use std::{marker::PhantomData, str::FromStr};

use common_utils::pii;

use super::*;
use crate::core::errors::ConnectorError;

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: types::PaymentsAuthorizeData {
amount: 1000,
currency: enums::Currency::USD,
payment_method_data: types::api::PaymentMethodData::Card(types::api::Card {
card_number: cards::CardNumber::from_str("4200000000000000").unwrap(),
card_exp_month: Secret::new("10".to_string()),
card_exp_year: Secret::new("2025".to_string()),
card_holder_name: Some(masking::Secret::new("John Doe".to_string())),
card_cvc: Secret::new("999".to_string()),
card_issuer: None,
card_network: None,
card_type: None,
card_issuing_country: None,
bank_code: None,
nick_name: Some(masking::Secret::new("nick_name".into())),
}),
confirm: true,
statement_descriptor_suffix: None,
statement_descriptor: None,
setup_future_usage: None,
mandate_id: None,
off_session: None,
setup_mandate_details: None,
capture_method: None,
browser_info: None,
order_details: None,
order_category: None,
email: None,
session_token: None,
enrolled_for_3ds: false,
related_transaction_id: None,
payment_experience: None,
payment_method_type: None,
router_return_url: Some("router_return_url".to_string()),
webhook_url: None,
complete_authorize_url: None,
customer_id: None,
surcharge_details: None,
request_incremental_authorization: false,
},
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,
}
}

#[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: "`pricing_type` not present in `CoinbaseConnectorMeta`"
},
);

// `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: "`pricing_type` not present in `CoinbaseConnectorMeta`"
},
);

// `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: "`pricing_type` not present in `CoinbaseConnectorMeta`"
},
);

// `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" : "blah" })
))))
.unwrap(),
CoinbasePaymentsRequest {
name: None,
description: Some("This is a test".to_string()),
pricing_type: "blah".to_string(),
local_price: LocalPrice {
amount: "1000".to_string(),
currency: "USD".to_string()
},
redirect_url: "router_return_url".to_string(),
cancel_url: "router_return_url".to_string(),
}
);
}
}
25 changes: 4 additions & 21 deletions crates/router/src/connector/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,7 @@ impl CardData for api::Card {
let year = self.get_card_expiry_year_2_digit();
Secret::new(format!(
"{}{}{}",
self.card_exp_month.peek().clone(),
self.card_exp_month.peek(),
bkontur marked this conversation as resolved.
Show resolved Hide resolved
delimiter,
year.peek()
))
Expand All @@ -796,14 +796,14 @@ impl CardData for api::Card {
"{}{}{}",
year.peek(),
delimiter,
self.card_exp_month.peek().clone()
self.card_exp_month.peek()
))
}
fn get_expiry_date_as_mmyyyy(&self, delimiter: &str) -> Secret<String> {
let year = self.get_expiry_year_4_digit();
Secret::new(format!(
"{}{}{}",
self.card_exp_month.peek().clone(),
self.card_exp_month.peek(),
delimiter,
year.peek()
))
Expand Down Expand Up @@ -1165,23 +1165,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 All @@ -1190,7 +1173,7 @@ where
{
let connector_meta_secret =
connector_meta.ok_or_else(missing_field_err("connector_meta_data"))?;
let json = connector_meta_secret.peek().clone();
let json = connector_meta_secret.expose();
json.parse_value(std::any::type_name::<T>()).switch()
}

Expand Down
2 changes: 1 addition & 1 deletion crates/router/src/connector/worldline/transformers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ fn make_card_request(
req: &PaymentsAuthorizeData,
ccard: &payments::Card,
) -> Result<CardPaymentMethod, error_stack::Report<errors::ConnectorError>> {
let expiry_year = ccard.card_exp_year.peek().clone();
let expiry_year = ccard.card_exp_year.peek();
bkontur marked this conversation as resolved.
Show resolved Hide resolved
let secret_value = format!(
"{}{}",
ccard.card_exp_month.peek(),
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 @@ -1661,9 +1661,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
7 changes: 3 additions & 4 deletions crates/router/src/core/fraud_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::fmt::Debug;

use api_models::{admin::FrmConfigs, enums as api_enums, payments::AdditionalPaymentData};
use error_stack::ResultExt;
use masking::PeekInterface;
use masking::{ExposeInterface, PeekInterface};
use router_env::{
logger,
tracing::{self, instrument},
Expand Down Expand Up @@ -167,10 +167,9 @@ where
match frm_configs_option {
Some(frm_configs_value) => {
let frm_configs_struct: Vec<FrmConfigs> = frm_configs_value
.iter()
.into_iter()
bkontur marked this conversation as resolved.
Show resolved Hide resolved
.map(|config| { config
.peek()
.clone()
.expose()
.parse_value("FrmConfigs")
.change_context(errors::ApiErrorResponse::InvalidDataFormat {
field_name: "frm_configs".to_string(),
Expand Down
2 changes: 1 addition & 1 deletion crates/router/src/core/payment_methods/vault.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl Vaultable for api::Card {
exp_month: self.card_exp_month.peek().clone(),
name_on_card: self
.card_holder_name
.clone()
.as_ref()
bkontur marked this conversation as resolved.
Show resolved Hide resolved
.map(|name| name.peek().clone()),
nickname: None,
card_last_four: None,
Expand Down
Loading