-
Notifications
You must be signed in to change notification settings - Fork 644
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
Correctly handle Cartes Bancaires test cards in retrievePossibleBrands #8258
Conversation
Diffuse output:
APK
DEX
|
:sonic: 👏🏻 |
private val CARTES_BANCAIRES_ACCOUNT_RANGES = setOf( | ||
BinRange( | ||
low = "2221000000000000", | ||
high = "2720000000000000" | ||
), | ||
BinRange( | ||
low = "4000000000000000", | ||
high = "4999999999999999" | ||
), | ||
BinRange( | ||
low = "5000000000000000", | ||
high = "5900000000000000" | ||
), | ||
BinRange( | ||
low = "6700000000000000", | ||
high = "6799999999999999" | ||
) | ||
).map { | ||
AccountRange( | ||
binRange = it, | ||
panLength = 16, | ||
brandInfo = AccountRange.BrandInfo.CartesBancaires | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do these bin ranges impact other cards besides the test cards? Is there any reason why we can't hardcode the test numbers rather than these ranges? In production the card metadata service determines what card brands to vend to the client, rather than have the client determine this. I'm concerned these bin ranges could have unintended consequences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do see failing tests relating to CBC and BIN in the bitrise run, I think we should hardcode these card numbers rather than have the test cards influence client side bin ranges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like StripeJS also hardcodes: https://git.corp.stripe.com/stripe-internal/stripe-js-v3/blob/4900dd4d/src/elements/inner/card/Bins.ts#L78
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can hardcode the test numbers. That seems good so that we can de-risk getting this fixed and to ensure we don't introduce any unintended behavior.
The BIN ranges that I added here match the regex that we use for CBC. This API and CBC don't actually use the same code path for determining possible brands. Created a jira to unify the two code paths because it seems less than ideal that this API would have different behavior than our implementation of CBC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated PR to just add the test numbers and no other ranges
@@ -43,17 +43,6 @@ class CardAccountRangeService( | |||
return | |||
} | |||
|
|||
val testAccountRanges = if (isCbcEligible()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was used to ensure that the test cards were handled properly in this service. Now the test cards are part of the static card ranges (queried below), so we don't need this special handling.
panLength = 16, | ||
brandInfo = AccountRange.BrandInfo.Visa, | ||
) | ||
fun `If CBC is disabled and only one brand matches, return the matched brand as soon as there is input`() = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test depends on there being only one card that matches the input. Now that the Cartes Bancaires test card also matches a "4" prefix, I needed to change the digit and card brand being used for this test
Summary
Return correct possible brands for Cartes Bancaires test cards in retrievePossibleBrands API
Motivation
Our API used to incorrectly return only Visa or only Mastercard for the two Cartes Bancaires multi-brand test cards, seen in our docs here
Testing
Changelog