-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Braintree: Pass overridden mid into client token for GS 3DS #5166
Braintree: Pass overridden mid into client token for GS 3DS #5166
Conversation
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 pretty much OK, @sinourain left some comments
@@ -35,8 +35,13 @@ def create_token_nonce_for_payment_method(payment_method) | |||
end | |||
|
|||
def client_token | |||
base64_token = @braintree_gateway.client_token.generate | |||
JSON.parse(Base64.decode64(base64_token))['authorizationFingerprint'] | |||
parameters = options[:merchant_account_id] ? { merchant_account_id: options[:merchant_account_id] } : {} |
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.
💬 Comment/Style:
nothing big:
base64_token = @braintree_gateway.client_token.generate({ merchant_account_id: options[:merchant_account_id] }.compact)
JSON.parse(Base64.decode64(base64_token)) | ||
end | ||
|
||
def authorization_fingerprint |
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.
❓ Question:
I get the point of calling it authorization_fingerprint
instead of client_token
but a method just get a key from a hash ?
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.
Make sense, I will remove authorization_fingerprint
assert_not_nil token | ||
end | ||
|
||
def test_client_token_generation_with_mid |
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.
❓ Question:
so Nraintree Sandbox doesn't care about a valid mid ?, you can just send a '1234' and they are going to provide you with a client token any way ?
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.
Correct, I tried passing multiple values and got a successful result with every field sent including the known field
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.
one general comment about the approach =>
Shouldn't the approach be to overwrite the mid
that is being sent from CORE to initialize the library so you don't have to think on modifing the mid on the client token creation ?, because I'm seeing the library being initialized with the original @options[:merchant_id]
so is possible to have the the instance intialized with mid 1234 and the create_token uses the mid 5678
@@ -21,7 +21,7 @@ def url | |||
def create_token_nonce_for_payment_method(payment_method) |
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'm pretty sure this method needs to be updated to take in options because where would we be getting options[:merchant_account_id] from
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.
good catch, I added the changes :)
c2e616b
to
9e4cbcd
Compare
good catch, I added the changes :) |
9e4cbcd
to
47949f1
Compare
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 think this looks good. I got stuck in the middle of the review trying to figure out what is happening on a certain method so I left a question.
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.
Thanks for pointing me to the SDK docs. This looks good to me!
47949f1
to
eb6bbb6
Compare
Summary: ------------------------------ Add merchant_account_id for the Client Token Generate and returns a string which contains all authorization and configuration information that the client needs to initialize the client SDK to communicate with Braintree [ECS-3617](https://spreedly.atlassian.net/browse/ECS-3617) [ECS-3607](https://spreedly.atlassian.net/browse/ECS-3607) Remote Test: ------------------------------ Finished in 6.262003 seconds. 7 tests, 15 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 1.12 tests/s, 2.40 assertions/s Unit Tests: ------------------------------ Finished in 38.744354 seconds. 5956 tests, 79952 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 153.73 tests/s, 2063.58 assertions/s RuboCop: ------------------------------ 798 files inspected, no offenses detected
eb6bbb6
to
c7269aa
Compare
Summary: ------------------------------ Add merchant_account_id for the Client Token Generate and returns a string which contains all authorization and configuration information that the client needs to initialize the client SDK to communicate with Braintree [ECS-3617](https://spreedly.atlassian.net/browse/ECS-3617) [ECS-3607](https://spreedly.atlassian.net/browse/ECS-3607) Remote Test: ------------------------------ Finished in 6.262003 seconds. 7 tests, 15 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 1.12 tests/s, 2.40 assertions/s Unit Tests: ------------------------------ Finished in 38.744354 seconds. 5956 tests, 79952 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 153.73 tests/s, 2063.58 assertions/s RuboCop: ------------------------------ 798 files inspected, no offenses detected Co-authored-by: Luis Urrea <devluisurrea+endava@gmail.com>
Summary: ------------------------------ Add merchant_account_id for the Client Token Generate and returns a string which contains all authorization and configuration information that the client needs to initialize the client SDK to communicate with Braintree [ECS-3617](https://spreedly.atlassian.net/browse/ECS-3617) [ECS-3607](https://spreedly.atlassian.net/browse/ECS-3607) Remote Test: ------------------------------ Finished in 6.262003 seconds. 7 tests, 15 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 1.12 tests/s, 2.40 assertions/s Unit Tests: ------------------------------ Finished in 38.744354 seconds. 5956 tests, 79952 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 153.73 tests/s, 2063.58 assertions/s RuboCop: ------------------------------ 798 files inspected, no offenses detected Co-authored-by: Luis Urrea <devluisurrea+endava@gmail.com>
…rchant#5166) Summary: ------------------------------ Add merchant_account_id for the Client Token Generate and returns a string which contains all authorization and configuration information that the client needs to initialize the client SDK to communicate with Braintree [ECS-3617](https://spreedly.atlassian.net/browse/ECS-3617) [ECS-3607](https://spreedly.atlassian.net/browse/ECS-3607) Remote Test: ------------------------------ Finished in 6.262003 seconds. 7 tests, 15 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 1.12 tests/s, 2.40 assertions/s Unit Tests: ------------------------------ Finished in 38.744354 seconds. 5956 tests, 79952 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 153.73 tests/s, 2063.58 assertions/s RuboCop: ------------------------------ 798 files inspected, no offenses detected Co-authored-by: Luis Urrea <devluisurrea+endava@gmail.com>
Summary:
Add merchant_account_id for the Client Token Generate and returns a string which contains all authorization and configuration information that the client needs to initialize the client SDK to communicate with Braintree
ECS-3617 ECS-3607
Remote Test:
Finished in 6.262003 seconds.
7 tests, 15 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
1.12 tests/s, 2.40 assertions/s
Unit Tests:
Finished in 38.744354 seconds.
5956 tests, 79952 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
153.73 tests/s, 2063.58 assertions/s
RuboCop:
798 files inspected, no offenses detected