refactor to Adyen::HPP #121
Conversation
…ulation for payment requests
… supported Ruby versions
| assert Adyen::HPP::Response.new(params, @shared_secret1).redirect_signature_check # explicitly provided shared secret | ||
|
|
||
| refute Adyen::HPP::Response.new(params.merge('skinCode' => 'sk1nC0de')).redirect_signature_check | ||
| refute Adyen::HPP::Response.new(params, @shared_secret2).redirect_signature_check # wrong shared secret |
There was a problem hiding this comment.
nitpick: here and in line 157, have you considered using variable names like @incorrect_secret and @correct_secret and dropping the comments?
There was a problem hiding this comment.
The globally defined secrets are either correct or incorrect depending on where (in combination with what skin code) they are used. I've added local variables with descriptive names.
|
Thanks @arnoutdemooij ! I've made a few inline comments. This looks like a great port of the current Adyen::Form functionality. For what it's worth, I was planning on taking a bit of a different direction, and taking the opportunity to be more backwards incompatible, making changes like keeping what are params in response/request separate from the shared secret for signing/verifying in method parameters, etc.. Of course talk is cheap, and you've provided the code :) Are you running your fork in production? How has it been working out for you? |
| HPP_URL % [domain, payment_flow.to_s] | ||
| end | ||
|
|
||
| def new_request skin=nil |
There was a problem hiding this comment.
Missing parentheses, and missing RDoc.
There was a problem hiding this comment.
Yes, I've added them.
|
Given that we are adding a completely new API, and we don't support Ruby 1.9 anymore, we could change some of these functions to use keyword arguments. I much prefer this over optional parameters (especially if there is many of them), or option hashes. Thoughts? |
|
@prognostikos @wvanbergen I did not want to move too far away from the original API, but I agree improvements can be made. Indeed it would be better to separate the secret key from the HPP parameter hash in the I think the distinction between an HPP client and an HPP request is somewhat artificial. Alternatively, each new request could be initialized with an environment, a skin (that supplies the secret key) and a set of HPP parameters, without the need for a separate client object. The environment and the skin would then be optionally provided and default to the values in the configuration. Also, Adyen has the notion of a single skin (one skin code) with an HMAC key per environment (test and live), which at the moment is not reflected by the API. I'm currently using my fork in an application that is still under development. It only uses Adyen's test environment. SHA-256 signed params are successfully POSTed to and accepted by Adyen and verification of the returned signature also goes well. |
|
I've made some changes. The secret key for an HPP request cannot be supplied as part of the payment parameters anymore. Instead, the secret of the skin that is supplied on initialization of the request is used. |
Conflicts: lib/adyen/hpp.rb
…onal skin, environment and/or shared secret
|
@prognostikos @wvanbergen The HPP client is now removed, because it didn't really add anything besides complexity. Furthermore, an HPP request now has to be initialized with a set of payment parameters. Optionally, a skin, an environment and a shared secret can be provided. If the skin is omitted, the default skin will be used. If the environment is omitted, the default or auto-detected environment is used. When the shared secret is omitted (which is recommend), the skin's secret is used for signing. All four attributes of a request ( Tests now fail for 'jruby-19mode', as a result of keyword arguments being used... |
…rguments, a feature introduced in Ruby 2.0.
|
I'm going to merge this so people can test the hpp branch for the coming update of Adyen to SHA-256. |
Refactor further to Adyen::HPP and, in this module, use SHA-256 signature calculation for payment requests. See issues #109 and #118.
SHA-1 signature calculation remains available (unchanged) in Adyen::Form.