Provide {:error, reason} on failure rather than exceptions
anthonator opened this issue · 4 comments
From an idiomatic Elixir point of view I think it makes sense to return an {:error, reason} tuple as opposed to throwing errors. I also think it would make it easier and more natural to consume the actual error if desired.
{:error, "no match of right hand side value: ['b']"} = Comeonin.Bcrypt.checkpw("a", "b")or maybe
{:error, :match_error, "no match of right hand side value: ['b']"} = Comeonin.Bcrypt.checkpw("a", "b")You could also provide ! functions for users that actually want exceptions thrown. This would be idiomatic as well.
Would be happy to help with this if needed.
Thanks for raising this point, but I'm afraid I disagree.
The exceptions are only raised when the types are wrong, and I think this should be caught early on, in development, and that is why an exception is raised.
I'm closing this issue for now, but feel free to add any comments / queries you have.
I think there are valid production use cases for passing nil or blank values to checkpw/2. Why is a nil or invalid hash an unexpected or exceptional situation? Is it a security concern?
Sorry I haven't got back to you earlier -- the day job has been keeping me really busy :(
In answer to your previous query, if the stored hash returns nil, then that means that the user has not been set up properly, or there is some other serious database issue. If the password is nil, I think you should catch that before the checkpw function -- the input should be validated beforehand.
If you are still worried about the password being nil, you could write password && checkpw(password, hash) instead of just checkpw(password, hash). In contrast, I think that changing the function to return true or {:error, message} would end up making your code more convoluted.
Finally, the reason Comeonin raises an error is because otherwise you would be faced with a rather cryptic Erlang error message when the hashing function fails.
Not a problem! I appreciate you taking the time to respond! 😄
A valid use case for having a nil password field could be that your app provides both password and social login. If a user has only a social login you can expect password to be nil. I think determining whether it's appropriate for a system to have a nil password should be beyond the scope of this project.
I disagree that returning {:error, message} would make code more convoluted. I think needing to catch exceptions is more convoluted and not idiomatic to Elixir. Returning {:error, message} provides the ability to pattern match and return a response appropriate to my app. As an alternative, you could also return false if checking the hash fails for any reason.
I'm not sure why I'd need to get a cryptic Erlang error. You could just return the same message that's currently being used in the exception.
At the end of the day, I really don't care if the password or hash is nil or just wrong for whatever reason. I just want to know it's wrong without having to jump through hoops. In my opinion, returning false would be perfect. If people want to know the reason then returning {:error, message} or {:error, reason, message} might be more appropriate. You could even continue to provide exceptions using ! functions (e.g. checkpw!).
P.S. I'm not trying to be a pain. I really like this library and want to provide my input/opinions to help make it better! 😃