Skip to content

Fix #70, Use SHA1 digest with pbkdf2 method - #72

Merged
ericelliott merged 3 commits into
ericelliott:masterfrom
sushantdhiman:fix-70
Jun 20, 2016
Merged

Fix #70, Use SHA1 digest with pbkdf2 method#72
ericelliott merged 3 commits into
ericelliott:masterfrom
sushantdhiman:fix-70

Conversation

@sushantdhiman

Copy link
Copy Markdown
Contributor

Fixes #70
Closes #71

Changelog

  • Added SHA1 digest for pbkdf2, will be ignored for v0.10
  • Fixed test to check non-empty error properly
  • Travis now run for v6

Essentially this would remove the deprecation warning in Node v6, but still remains a non breaking change because of version check.

cc @ericelliott @tjconcept @brianmhunt

* Added SHA1 digest method to pbkdf2
* fixed inconsistent error message for non-empty strings
* test fixed for correct error message comparision
@sushantdhiman

Copy link
Copy Markdown
Contributor Author

https://github.com/ericelliott/credential/blob/master/test/credential-test.js#L248 test is failing as I may have hit the 0.01% chance for it to fail. But its more like a timing issue I think.

@sushantdhiman sushantdhiman changed the title Fix #70, Use SHA1 digest with with pbkdf2 method Fix #70, Use SHA1 digest with pbkdf2 method Jun 11, 2016
Comment thread test/cli-test.js Outdated
var expected = /Error: Password must be a non-empty string/;

t.is(actual, expected);
t.throws(function (){

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why not test against the stderr.message property? As you have demonstrated here, .throws() is for testing whether or not a function throws. What we really want to test is whether or not we get the expected error message, right?

@sushantdhiman sushantdhiman Jun 13, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought it would be better to use tape api to check against errors. But as we already have stderr stream, we can use it to make it simple. Will make this change soon

@ericelliott ericelliott Jun 13, 2016

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We're still using the tape API when we check against the stderr.message property. Again, .throws() is for testing whether or not a function throws. Note how you had to create a fake function and test against that to use it... That just forces us to look harder to see what you're really trying to test...

@sushantdhiman

sushantdhiman commented Jun 13, 2016

Copy link
Copy Markdown
Contributor Author

@ericelliott changed the test. I have used pattern match just like #71 . Its simpler than empty function and throws, waiting on CI now. Any other change?

@sushantdhiman

Copy link
Copy Markdown
Contributor Author

Hi @ericelliott , Do you need any improvements in this. I could really use this update in my projects :)

@ericelliott
ericelliott merged commit daa8b6a into ericelliott:master Jun 20, 2016
@sushantdhiman
sushantdhiman deleted the fix-70 branch June 20, 2016 05:28
@boutell

boutell commented Sep 1, 2016

Copy link
Copy Markdown

Can this be published as 1.0.1? The deprecation warning is unfortunate when making the case that this is the "right" module for the job. Thanks!

@sushantdhiman

Copy link
Copy Markdown
Contributor Author

#73

zemirco pushed a commit to zemirco/couch-pwd that referenced this pull request Mar 28, 2017
* Add explicit SHA1 digest in crypto.pbkdf2 calls

Deprecation mentioned in ericelliott/credential#70, implemented in ericelliott/credential#72.

* Update node_js versions in .travis.yml

Source: https://docs.travis-ci.com/user/languages/javascript-with-nodejs/
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.

3 participants