Pull Request Test Coverage Report for Build 213

• 49 of 50 (98.0%) changed or added relevant lines in 3 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-0.3%) to 95.792%

Totals
Change from base Build 207:	-0.3%
Covered Lines:	478
Relevant Lines:	499

💛 - Coveralls

Hello,

After playing with it a bit, I have some question.

Why does the Verifier needs a Version? It should be able to infer it from the hash ($2y$, $2a$...).
The version should only be needed for VerifyStrict if one wants to explicitly restrain the version allowed.

I think the length for the TruncateStrategy should not be tied to the Version. It should be independent.

What do you think?

Why does the Verifier needs a Version?

Thats a good point. The problem is that the version identifier is not unique in my lib anymore. I will think about it

I think the length for the TruncateStrategy should not be tied to the Version. It should be independent.

They are independent, they are just using it as configuration by default, which makes sense (why would you want a differnt lenght for those 2 properties?)

They are independent, they are just using it as configuration by default, which makes sense (why would you want a differnt lenght for those 2 properties?)

I don't understand this sentence, sorry.

Correct me if I'm wrong, but now the LongPasswordStrategies interface expects a Version parameter, and nothing else.
In LongPasswordStrategies.java:

   public static LongPasswordStrategy truncate(BCrypt.Version version) {
        return new LongPasswordStrategy.TruncateStrategy(Objects.requireNonNull(version).allowedMaxPwLength);
    }
    public static LongPasswordStrategy hashSha512(BCrypt.Version version) {
        return new LongPasswordStrategy.Sha512DerivationStrategy(Objects.requireNonNull(version).allowedMaxPwLength);
    }

LongPasswordStrategies.truncate() should expects a length.
LongPasswordStrategies.hashSha512() should expects... nothing really. This strategy is supposed to allow user to stop caring about password length.

Correct me if I'm wrong, but now the LongPasswordStrategies interface expects a Version parameter, and nothing else.

Ah yes, you are absolutely right - to me it makes the most sense that the length is derived from the version. The developer is not tied to use the LongPasswordStrategies, he/she can implement them themselves. But I think about adding one with int signature.

Ah, I can see your point. But I fear that it's not that obvious for everyone, and that it is not close to what one would expect.

For instance in my case it wouldn't work. I use different libs that all needs to truncate to 72, but that vary by version (one is generating $2a$, another one $2y$).
So I should perform string replace on the version prefix and so on...

Yes it's really a mess.

For instance in my case it wouldn't work. I use different libs that all needs to truncate to 72, but that vary by version (one is generating $2a$, another one $2y$).
So I should perform string replace on the version prefix and so on...

I'm afraid I don't really understand the problem.

Current state:

• I refactored the verifier to either derive the version from the hash or use a version that is provided. I will keep the API interface as it is for now (passing the version through verifier constructor)
• The LongPasswordStrategies factory class will only accept Versions because I still think it makes the most sense, but anybody can use the implementations without the factory, eg. new LongPasswordStrategy.TruncateStrategy(72); so you are free to define whatever you want

Alright! Works for me 👍
Thank you!

Thanks @Indigo744 your input was very valuable for this release. 0.9.0 will be out in a couple of hours.