search

waterlock / waterlock-local-auth

Local authentication method for waterlock
http://waterlock.ninja/
MIT License
39 stars 83 forks source link

change Bcrypt for bcryptjs #32

Open mdepecker opened 9 years ago

mdepecker commented 9 years ago

bcryptjs is API compatible to bcrypt but 100% pure JS, which avoids issues with the node-gyp compiling, for example having the wrong python version and similar issues.

Basically, it is the same thing but just JS, so no issues caused by the native stuff, which makes for better forward and backwards compatibility and an easier time when deploying. Always bet on JS™ :)

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.44%) to 39.33% when pulling ff786b3488333dec4fc18cafbe12a4e98155abf6 on mdepecker:master into 2897e12d240035b2b58ed114260bf82194192acf on waterlock:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.44%) to 39.33% when pulling ff786b3488333dec4fc18cafbe12a4e98155abf6 on mdepecker:master into 2897e12d240035b2b58ed114260bf82194192acf on waterlock:master.

wayne-o commented 9 years ago

Is this the preferred way to do this? I've read bad things perf wise but have zero numbers to back this up!

Are we wanting to go down the JS compat route to eliminate native code?

If we do then i can clean up and merge in.

s-devaney commented 9 years ago

Yes. In order to get waterlock-local-auth working on Windows with current native bcrypt implementation I had to install Python (~200mb?) and Windows Visual Studio (~4GB) which is just ridiculous.

If bcryptjs is a performance concern my recommendation would be to use npm credential, but that is outside of the scope of this PR.

mdepecker commented 9 years ago

@s-devenay

That's exactly the point that I try to pointed out. I don't know about credential But just for the sake of our sanity and the easy to use stuff this could be great

kriswill commented 9 years ago

I think this is fine, especially since bcrypt updating to latest nan is so slow. rebase and squash, and I'll merge it.

yura415 commented 9 years ago

So any chances that this will be accepted? Can't lift my app on production server 'cause can't install newer version of GCC on FreeBSD.

mdepecker commented 9 years ago

Sorry , didn't really have time to handle I'll rebase this and wait for merge @yura415 For a quick fix you can just check my own version and see what change then do the same, if this will be merge just do a npm update