hs-bcrypt icon indicating copy to clipboard operation
hs-bcrypt copied to clipboard

Newtype for hash values

Open fisx opened this issue 9 years ago • 3 comments

I spent some resources on finding a mistake I made that would have been caught by the type checker with this change.

I haven't tested this, but would like to hear your opinion before I do: Any objections? Would it be if I started an hspec test suite? Should I factor out an internal module so we can reduce the export list while still allowing people to get access to everything if they really want to?

(Thanks @np!)

fisx avatar Jan 21 '16 15:01 fisx

I think we should let this one wait a bit and get some more feedback from the community about how they use hs-bcrypt.

On one hand, some (most?) users will probably immediately want to put the result into a library that needs a ByteString (e.g. to save in a file or an SQL database of some form) - with this change, they need to deconstruct the BCryptHash (and reconstruct when they load it back). It is trivial to do, but it is a change from the status quo and some users might find it annoying.

On the other hand, some users will want to pass it around further; this probably applies more for those building more complex applications, and would benefit from the additional type safety. Those applications could define the newtype themselves - it doesn't necessarily have to be provided by the library, but providing it in the library prevents the extra step, and if there are ever libraries that use hs-bcrypt and take a password hash, having a standard newtype would mean they all use the same one.

On your other comments: I think a test suite would be a great idea. I'm not so sure about reducing the import list - everything that is exported is designed to be used by routinely by applications; hashUsesPassword etc... are only needed once you need to 'upgrade' a password to a stronger policy on login, which new applications won't need. Given that it is important to review and upgrade to appropriate hashing policies as available computing power changes, I wouldn't want to discourage people from writing policy upgrade code by placing those exports in an internal module.

I would welcome feedback from other users of hs-bcrypt on whether they would find the newtype useful.

A1kmm avatar Jan 26 '16 00:01 A1kmm

Thanks for the thorough feedback! After reading this I am still in favor of the newtype, but I'll still be a happy user no matter what the outcome of this discussion. (-:

If the decision is for the newtype, I will write some hspec tests.

Not sure about the Internal module any more. I think I was aiming for an abstract newtype, but as you say serialization will require all users to deconstruct that.

Let's hear what others have to say.

fisx avatar Jan 26 '16 04:01 fisx

it's a good patch and it's something i wanted to do in #10

dredozubov avatar Apr 27 '16 13:04 dredozubov