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

Add docs to Crypto::Bcrypt #9647

Merged

Conversation

j8r
Copy link
Contributor

@j8r j8r commented Jul 30, 2020

This method is untested, and is a little variation of the "standard" Crypto::Bcrypt.new constructor.

There are also other points I think can be improved in Crypto::Bcrypt, I have kept this PR minimal.

Only adding docs on this PR.

@j8r j8r changed the title Deprcate Crypto::Bcrypt.hash_secret Deprecate Crypto::Bcrypt.hash_secret Jul 30, 2020
@j8r j8r force-pushed the deprecate-bcrypt.hash_secret branch 2 times, most recently from 8198294 to c3af0ce Compare July 30, 2020 19:59
@straight-shoota
Copy link
Member

I'm not sure about this. hash_secret seems like a nice convenience method if you just want a simple string hash with no seed.
The seed is not optional for Crypto::Bcrypt.new, though. It's a more advanced interface and IMO it makes sense to force users to think about it. Also it's probably not a good idea to mix constructor arguments accepting String and Byte for password and salt.

@j8r
Copy link
Contributor Author

j8r commented Jul 30, 2020

@straight-shoota For a more advanced wrapper, there is Crypto::Bcrypt::Password (which could IMO be included in Crypto::Bcrypt by the way).

@j8r
Copy link
Contributor Author

j8r commented Jul 30, 2020

I think it is great for the user to know what is the default salt value in the API docs. I can split to two overload though, one for Bytes and the other for String.

@j8r
Copy link
Contributor Author

j8r commented Jul 30, 2020

An other option is to merge Crypto::Bcrypt::Password into Crypto::Bcrypt. Do we want this change now?

@straight-shoota
Copy link
Member

straight-shoota commented Nov 15, 2020

I actually don't see any benefit from this.

hash_secret is a convenience method based on Bcrypt for simple uses. Being a separate method you can't accidentally "forget" to pass a salt when initializing an instance with Bcrypt.new because the compiler enforces it.
If we go with this patch, all usages of Bcrypt.hash_secret would need to be replaced by Bcrypt.new.to_s. And the entire API would be more susceptible to mistakes because you could always omit the salt.

The method not being tested can be fixed by adding a test. The added documentation is also great. But I wouldn't change the API because it's exactly like it should be.

@straight-shoota
Copy link
Member

Regarding Bcrypt::Password, please create a new issue.

@j8r
Copy link
Contributor Author

j8r commented Nov 15, 2020

I don't agree @straight-shoota , because one could use .new instead of .hash_secret, by mistake (or not). It is not obvious which one is "safer" than the other.

@j8r
Copy link
Contributor Author

j8r commented Nov 15, 2020

Anyway, ok. I'll do the non-controversial changes here, first, and open an issue for broader API improvements about the Bcrypt class.

@straight-shoota
Copy link
Member

When calling Bcrypt.new, salt is a required argument. You can't just miss it by mistake.

@j8r j8r force-pushed the deprecate-bcrypt.hash_secret branch 2 times, most recently from f082b3f to d499863 Compare November 15, 2020 13:59
@j8r
Copy link
Contributor Author

j8r commented Nov 15, 2020

I didn't add a test, because hash_secret will require Crypto::Bcrypt::Password to check the values.

@straight-shoota
Copy link
Member

That's probably fine. It's tested indirectly through the specs for Password.create.

src/crypto/bcrypt.cr Outdated Show resolved Hide resolved
@j8r j8r changed the title Deprecate Crypto::Bcrypt.hash_secret Add docs to Crypto::Bcrypt Nov 15, 2020
src/crypto/bcrypt.cr Outdated Show resolved Hide resolved
src/crypto/bcrypt.cr Outdated Show resolved Hide resolved
src/crypto/bcrypt.cr Outdated Show resolved Hide resolved
@j8r j8r force-pushed the deprecate-bcrypt.hash_secret branch from 8bf9415 to 08af313 Compare September 16, 2021 18:25
@j8r
Copy link
Contributor Author

j8r commented Sep 16, 2021

These are now just docs, can we move forward please?

@j8r
Copy link
Contributor Author

j8r commented Dec 6, 2021

Anything blocking the PR?

@oprypin oprypin self-requested a review December 6, 2021 22:36
@straight-shoota straight-shoota added this to the 1.3.0 milestone Dec 6, 2021
@straight-shoota straight-shoota changed the title Add docs to Crypto::Bcrypt Add docs to Crypto::Bcrypt Dec 8, 2021
@straight-shoota straight-shoota merged commit d72f6ea into crystal-lang:master Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants