dwyl / abase

:house: A (solid) Base for your Web Application.
9 stars 0 forks source link

Hashing Data #58

Closed jrans closed 7 years ago

jrans commented 7 years ago

THIS PR

Necessary for password, secret answers etc. Function can be used at layer most appropriate

Related: #52

jrans commented 7 years ago

CI needs fixing but code itself passes on my machine and so can start your review while I fix, will remove WIP when done

jrans commented 7 years ago

DONE Thanks to https://github.com/ncb000gt/node.bcrypt.js/issues/366

jrans commented 7 years ago

@nelsonic Yes salt number should be configurable. Will change.

This function does indeed only serve purpose for saving data that needs to be hashed.

Actually using that hash to authenticate a login must be done separately but will be done later with #53

codecov-io commented 7 years ago

Current coverage is 100% (diff: 100%)

Merging #58 into master will not change coverage

@@           master   #58   diff @@
===================================
  Files          10    11     +1   
  Lines         174   202    +28   
  Methods        38    46     +8   
  Messages        0     0          
  Branches       25    30     +5   
===================================
+ Hits          174   202    +28   
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last update d6d0fc9...0bcc894

nelsonic commented 7 years ago

@jrans what I'm saying is that given that the purpose of this PR is to hash ("sensitive") data and the only situation where we need to hash (i.e. one-way "encrypt") data is for passwords which we need the plaintext for in order to use bcrypt.compare (unless there is another MVP usecase I'm not aware of) the functionality becomes YAGNI so PR could be closed

image

the feature/idea should be documented in the issue so we can come back to it if/when we have a clear use-case

jrans commented 7 years ago

@nelsonic Maybe you're missing what I'm proposing but this is function is used on insertion of a new user and password into the database. If the field is meant to be a password it will hash the data if not then it will leave it as is.

Yes the only field we need to hash is "password", but what if we had two password fields? What if we wanted to store secret answers? Although this is maybe above and beyond MVP its functionality that can be used in future.

Two options:

Realise the latter is one more step for the user and we're probably assuming email, password and username necessary for all user data so this could be done by us anyway when applying default requirements on the config.

jrans commented 7 years ago

@nelsonic ok closing