Skip to content
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

Add HMAC support to Crypto module #256

Merged
merged 3 commits into from
Jun 20, 2017

Conversation

edgardoalz
Copy link
Contributor

Closes #231

@@ -151,3 +152,38 @@ func (hasher *Hasher) Digest(outputEncoding string) string {

return ""
}

func (c Crypto) CreateHmac(ctx context.Context, algorithm string, key string) *Hasher {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be capitalized; createHMAC() rather than createHmac(). It would be inconsistent with hmac(), but I think that'll have to stay for consistency's sake...


rt.Set("crypto", common.Bind(rt, &Crypto{}, &ctx))

t.Run("Valid", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add test cases for an invalid algorithm too, and use the testdata/t.Run() idiom to run hasher + wrapper tests for each supported algorithm: https://github.com/loadimpact/k6/blob/0092f27224b995c3bfd41a7b9cc756dfef8f6777/js/modules/k6/k6_test.go#L49

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote a default case in createHmac method for invalid or not supported algorithms that throw an "Invalid algorithm error", and I added it to unit tests, I think createHash must have the same default case, because trying to update or digest on the hasher created will cause a runtime error with nil pointer dereference, because hasher.hash is never asigned.

@liclac
Copy link
Contributor

liclac commented Jun 19, 2017

Almost there!

@liclac
Copy link
Contributor

liclac commented Jun 20, 2017

Just get that function name changed and I'll merge o7

@edgardoalz
Copy link
Contributor Author

I just changed it

@liclac
Copy link
Contributor

liclac commented Jun 20, 2017

👍

@liclac liclac merged commit 2d9184a into grafana:master Jun 20, 2017
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