Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation is confusing and could use improvement and/or restructuring #38

Closed
egeersoz opened this issue Dec 7, 2019 · 4 comments
Closed

Comments

@egeersoz
Copy link

egeersoz commented Dec 7, 2019

First of all, thank you for your work. I'm really glad this exists as an alternative to Bcrypt.

I've been trying to wrap my mind around the various functions and behaviors implemented by this library, and I'm having a bit of trouble. Since authentication logic is important and misunderstandings can potentially result in severe security flaws, I want to make sure I understand everything thoroughly, and that other people do, as well.

To start with an example: what is the difference between check_pass and verify_pass? The function names suggest they are identical, but based on the examples they are different. Is there a reason both of these exist, when they seem to do the same thing (albeit with different param types)? When is a good time to use one or the other? I looked at the source code -- Comeonin's check_pass calls verify_pass internally, and that returns a true or false. But Argon2's verify_pass can additionally raise an ArgumentError...

Similarly, there seems to be a lot of redundancy between add_hash and hash_pwd_salt. Again, I understand that add_hash returns a map that is meant to be used in an Ecto changeset, and it works fine for that purpose. But the way these functions are named makes the reader wonder whether add_hash uses a salt as well (I had to read the source code to verify, just for ease of mind). Maybe rename it to add_hash_salt, or even better, add_hash_salt_to_changeset, since that's what it does? (I also wish there was further standardization in function names - some functions refer to passwords as "pass" and others refer to them as "pwd" - just a thought.)

Generally, the organization of the documentation on Hex.pm is a bit confusing, and seems non-standard. Information about various functions is scattered under 'Options' and 'Examples' headers, and the 'Functions' header contains only a single function, gen_salt. This makes it challenging to understand the proper and recommended usage of each function. For example, I was trying to find the possible returns of check_pass, and after a lot of browsing around I ended up having to read the source code of Comeonin itself, despite the fact that the function is called from Argon2. I get that it's a behavior, but properly organized documentation that clearly shows param and return types would eliminate the need to read the source code. Adding hyperlinks to the relevant sections of the Comeonin docs would also help. :)

I know that the Comeonin framework has gone through multiple iterations and redesigns, so maybe things are still in flux. Once I fully grok this library and how to properly use it in a "real-life" project, I'll submit a tutorial with a reference project link so that it can be added to the docs. I just wanted to submit this piece of feedback so that you're aware of the difficulties users might be facing. Thank you.

@riverrun
Copy link
Owner

Thanks for raising the issue.

Some of the problems with organization is that argon2_elixir use-s functions defined in Comeonin, but the function documentation cannot be accessed directly via argon2_elixir, and so I put the documentation for those functions in the module documentation.

One other problem is that these libraries are very widely used, and it is difficult to change the names of functions without causing confusion / problems for users when they update the library.

Anyway, I will have a further look at this issue in the new year and get back to you then.

@riverrun
Copy link
Owner

One more thing: for most apps, you will just need to use the add_hash and check_pass functions, which are convenience functions. Three are examples of how to use them in the module docs, and I will look at making it clearer that those are the only functions that most users will need to use.

@riverrun
Copy link
Owner

Updated the documentation in 2.2.0 - the hex docs can be seen here.

If you have any comments, queries or suggestions, please let me know.

@riverrun
Copy link
Owner

Closing this issue for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants