Minor updates for timestamp, nonce and digest functions, namespace changes

Mastercard / oauth1-signer-ruby

Zero dependency library for generating a Mastercard API compliant OAuth signature.

https://developer.mastercard.com/platform/documentation/security-and-authentication/using-oauth-1a-to-access-mastercard-apis/

MIT License

5 stars 0 forks source link

Minor updates for timestamp, nonce and digest functions, namespace changes #21

Closed amasses closed 1 year ago

amasses commented 1 year ago

PR checklist

[x] An issue/feature request has been created for this PR
[x] Pull Request title clearly describes the work in the pull request and the Pull Request description provides details about how to validate the work. Missing information here may result in a delayed response.
[x] File the PR against the master branch
[x] The code in this PR is covered by unit tests

Link to issue/feature request

https://github.com/Mastercard/oauth1-signer-ruby/issues/20

Description

All items below fix #20

Removed unnecessary call to getutc on Time object. Unix timestamps are based on UTC so this is unnecessary
Replaced code for get_nonce with SecureRandom calls. The previous code was deterministic and provided no guarantees of randomness
Replaced call to Digest::SHA256 with OpenSSL::Digest as Digest is falling out of favour (and openssl is already being loaded and used)

danny-gallagher commented 1 year ago

Hi @amasses,

Thanks for creating these PRs, however from the file diff we cannot see the changes you've mentioned in the description. Can you have a look?

amasses commented 1 year ago

Hi @danny-gallagher

In commit 89d7a4e it is pretty clear - I've highlighted the changes in the following screenshot:

danny-gallagher commented 1 year ago

Hi @danny-gallagher

In commit 89d7a4e it is pretty clear - I've highlighted the changes in the following screenshot:

If I look at the branch of your fork you're trying to merge in, I don't see any of the changes you've mentioned above. For example, I don't see the new "securerandom" import here.

https://github.com/amasses/oauth1-signer-ruby/blob/minor-fixes/lib/mastercard/oauth.rb

amasses commented 1 year ago

If I look at the branch of your fork you're trying to merge in, I don't see any of the changes you've mentioned above. For example, I don't see the new "securerandom" import here.

https://github.com/amasses/oauth1-signer-ruby/blob/minor-fixes/lib/mastercard/oauth.rb

@danny-gallagher sorry I am not sure what went on there - the commit somehow disappeared (despite that screenshot being taken from the PR... Github weirdness).

I have re-committed the changes, so hopefully you can see them now.