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

feat(connector): add threedsecureio three_ds authentication connector #4004

Merged
merged 28 commits into from
Mar 9, 2024

Conversation

sai-harsha-vardhan
Copy link
Contributor

@sai-harsha-vardhan sai-harsha-vardhan commented Mar 7, 2024

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

add threedsecureio three_ds authentication connector

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Motivation and Context

How did you test it?

Compiler guided and payments sanity testing

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code
  • I added unit tests for my changes where possible
  • I added a CHANGELOG entry if applicable

@sai-harsha-vardhan sai-harsha-vardhan added A-connector-integration Area: Connector integration C-feature Category: Feature request or enhancement S-waiting-on-review Status: This PR has been implemented and needs to be reviewed labels Mar 7, 2024
@sai-harsha-vardhan sai-harsha-vardhan added this to the March 2024 milestone Mar 7, 2024
@sai-harsha-vardhan sai-harsha-vardhan self-assigned this Mar 7, 2024
@sai-harsha-vardhan sai-harsha-vardhan requested review from a team as code owners March 7, 2024 10:40
) -> CustomResult<RequestContent, errors::ConnectorError> {
let connector_router_data = threedsecureio::ThreedsecureioRouterData::try_from((
&self.get_currency_unit(),
req.request
Copy link
Contributor

Choose a reason for hiding this comment

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

When will Authentication flow comes into picture? After payment create or before?

If we call this flow after payments create then why amount and currency has to be option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be the common flow for both payment and non-payment authentication (payment_method authentication). payment_method authentication is yet to be implemented, which doesn't have amount and currency in the authentication flow

))?;
let req_obj =
threedsecureio::ThreedsecureioAuthenticationRequest::try_from(&connector_router_data);
println!("req_obj authn {:?}", req_obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove print statements

.parse_struct("ThreedsecureioAuthenticationResponse")
.change_context(errors::ConnectorError::ResponseDeserializationFailed)?;
event_builder.map(|i| i.set_response_body(&response));
types::RouterData::try_from(types::ResponseRouterData {
Copy link
Contributor

Choose a reason for hiding this comment

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

add logger for response

.threeds_server_transaction_id
.clone(),
};
println!("req_obj post-authn {:?}", req_obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove

.response
.parse_struct("threedsecureio ThreedsecureioPreAuthenticationResponse")
.change_context(errors::ConnectorError::ResponseDeserializationFailed)?;
event_builder.map(|i| i.set_response_body(&response));
Copy link
Contributor

Choose a reason for hiding this comment

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

also add logger for response

},
)?;
let billing_state = billing_address.clone().to_state_code()?;
let billing_country = isocountry::CountryCode::for_alpha2(
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this conversion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to get the numeric_id of the country (https://docs.rs/isocountry/latest/isocountry/numeric/index.html), that's the reason we are using isocountry

.expose()
.to_string(),
bill_addr_state: billing_state.peek().to_string(),
three_dsrequestor_authentication_ind: "01".to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

value always going to be 01? For hard coded values like this please add comments about the significance.

}
.to_string(),
browser_javascript_enabled: browser_details
.clone()
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid repeated clone of browser_details instead clone the respective values

.to_string(),
purchase_date: date_time::DateTime::<date_time::YYYYMMDDHHmmss>::from(date_time::now())
.to_string(),
sdk_app_id: sdk_information.clone().map(|sdk_info| sdk_info.sdk_app_id),
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid repeated clone of sdk_information instead clone the respective values

@@ -120,6 +120,8 @@ openapi = { version = "0.1.0", path = "../openapi", optional = true }
erased-serde = "0.3.31"
quick-xml = { version = "0.31.0", features = ["serialize"] }
rdkafka = "0.36.0"
isocountry = "0.3.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not necessary, As we are already using iso codes in our enum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is required to get numeric_id of the country

pub three_dscomp_ind: ThreeDSecureIoThreeDsCompletionIndicator,
pub three_dsrequestor_url: String,
pub acquirer_bin: String,
pub acquirer_merchant_id: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please mask the necessary PII information

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not pii

Copy link
Contributor

Choose a reason for hiding this comment

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

Please recheck all the fields and mask the necessary

pub device_channel: String,
pub browser_javascript_enabled: Option<bool>,
pub browser_accept_header: Option<String>,
pub browser_ip: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Use type Secret<String, IpAddress>


#[derive(Debug, Serialize, Deserialize)]
pub struct ThreeDSecureIoMetaData {
pub mcc: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Mask the necessary fields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not pii

ArjunKarthik
ArjunKarthik previously approved these changes Mar 8, 2024
Base automatically changed from add-new-payment-apis-for-external-authn to main March 9, 2024 07:30
@Gnanasundari24 Gnanasundari24 dismissed ArjunKarthik’s stale review March 9, 2024 07:30

The base branch was changed.

@sai-harsha-vardhan sai-harsha-vardhan requested a review from a team as a code owner March 9, 2024 07:30
@likhinbopanna likhinbopanna added this pull request to the merge queue Mar 9, 2024
Merged via the queue into main with commit 06c3096 Mar 9, 2024
10 of 12 checks passed
@likhinbopanna likhinbopanna deleted the add-threedsecureio-3ds-connector branch March 9, 2024 09:33
@SanchithHegde SanchithHegde removed the S-waiting-on-review Status: This PR has been implemented and needs to be reviewed label Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-connector-integration Area: Connector integration C-feature Category: Feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants