Skip to content

Conversation

YomesInc
Copy link
Contributor

No description provided.

@YomesInc YomesInc requested a review from nitzanj July 20, 2020 15:57
* @return hex-string representation of signature calculated based on provided parameters map and secret
*/
public static String produceSignature(Map<String, Object> paramsToSign, String apiSecret) {
public static String produceSignature(Map<String, Object> paramsToSign, String apiSecret, Signer algorithmType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't just add a parameter to a public method, we need to keep it backward compatible. To make it viable you should add an overload without the algorithm param, and delegates to this method with the default algorithm (SHA-1)

public AuthToken authToken;
public boolean forceVersion = true;
public boolean longUrlSignature = DEFAULT_IS_LONG_SIGNATURE;
public Signer signatureAlgorithm = DEFAULT_SIGNATURE_ALGORITHM;
Copy link
Contributor

Choose a reason for hiding this comment

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

This Signer class is a bit problematic - It's either a signer or a signing algorithm. It can't be both.
We need a SignatureAlgorithm enum. The Signer class itself is a bit unnecessary.

Whenever an algorithm is passed around or stored (e.g. in config) it needs to be the enum, not a signer class.

import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;

public class Signer {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this class, as explained above. It tries to be both the data and the logic. There should just be an enum for the data. That enum can have a string value attached (in a format that MessageDigest recognizes).
An enum can also easily load from string - no need to write your own getByName()

And this class becomes a two-line method in utils (and it gets the signature algorithm enum as a parameter).

} catch (NoSuchAlgorithmException e) {
throw new RuntimeException("Unexpected exception", e);
}
Signer signer = longUrlSignature ? Signer.SHA256 : config.signatureAlgorithm;
Copy link
Contributor

Choose a reason for hiding this comment

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

So again, this should be the algorithm enum, not a signer class. And then when it's used in line 396 below, just call the signing method sending it the algorithm.

@nitzanj nitzanj merged commit 6be288b into master Aug 2, 2020
@nitzanj nitzanj deleted the feature/add-sha256-auth-signature-support branch October 28, 2020 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants