search

cdepillabout / password

datatypes and functions for easily working with passwords in Haskell
http://hackage.haskell.org/package/password
55 stars 16 forks source link

Do not export the constructor of Pass #5

Closed Vlix closed 4 years ago

Vlix commented 4 years ago

To make it more safe, it'd be better that the Pass data constructor not be exported. This way, users can't pattern match on it and just remove the Text without using a function that's clearly labeled as unsafe.

Obviously, accompanying this should be a function like mkPass :: Text -> Pass to have users still be able to produce a Pass.

cdepillabout commented 4 years ago

This seems like a good idea.

Maybe in the future, we could export the constructor in a module like Data.Password.Internal, but for now just hiding the constructor seems like a good idea.

Vlix commented 4 years ago

Also, I'm wondering what the added value is of having a Read instance for Pass. If it has an IsString instance, you can just fromString foo where foo :: String. Any read function is also from a String, so it's superfluous AND because it makes more sense to implement a Show function (see below), you'd rather not have a Read and Show that are not isomorphic.

Defining the Show instance as follows:

instance Show Pass where
  show _ = "**PASSWORD**"

This way no one can make their own Show function for Pass (maybe without thinking it through, just to make something type check) and forcing the user, again, to use a function labeled unsafe to get to print the password.

cdepillabout commented 4 years ago

I am kind of on the fence with having a Show instance for Pass.

On one hand, I don't want people to ever get lax about just showing a Pass. I'd like users to explicitly have to use the unsafe functions if they want to go from Pass to String (even if show doesn't actually show the password).

On the other hand, it is really annoying when types don't implement a Show instance. It becomes very slightly harder to implement a Show instance for any record containing a Pass.

I guess I'd be okay with adding a Show instance as long as there is a warning in the documentation about exactly what it does.

you'd rather not have a Read and Show that are not isomorphic

I don't really buy into the Read / Show isomorphism law. Especially in cases like this where it could make sense to keep them different.

Now that I've taken a look at it, I think the bigger problem is that there is no documentation about what format the Read function is expecting. And it is hard to figure out because there is also no show instance.

To sum up, I guess I don't really care about Show and Read, but if they do exist, they should have good documentation!