Next Major Release - A Suggestion

[nicklog]

Hello,

It's me again ;)

Actually this PR shouldn't be so big, but somehow it happened.

I would implement this library into my own projects, but wanted to use my own HTTP client and cache provider. This was not possible in the original version. So I made some changes to make it possible. Some new dependencies need PHP7 to run. That's why I increased the minimum version to 7.1 in this PR. Additionally I added some help functions and an abstract class and interface. This allows developers to make their own implementations as needed.

Think of it as a kind of beta for version 3. I haven't adapted the Readme yet.
Just check it out and give me feedback.

If you would accept the PR i would write a Symfony bridge to ;)

Best regards.

[coveralls]

Pull Request Test Coverage Report for Build 126

• 70 of 79 (88.61%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.3%) to 89.024%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/PasswordExposedChecker.php	36	40	90.0%
src/AbstractPasswordExposedChecker.php	32	37	86.49%

Totals
Change from base Build 124:	-0.3%
Covered Lines:	73
Relevant Lines:	82

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 143

• 79 of 90 (87.78%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-1.3%) to 88.043%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/PasswordExposedChecker.php	41	46	89.13%
src/AbstractPasswordExposedChecker.php	36	42	85.71%

Totals
Change from base Build 124:	-1.3%
Covered Lines:	81
Relevant Lines:	92

---

💛 - Coveralls

[DivineOmega]

Hello. Thanks for the PR.

However, I'm a little concerned that this seems very complex for just allowing developers to use a custom client and cache.

I have implemented the ability to use a custom PSR-6 cache library in #23. I think this is somewhat simpler.

In regards to allowing a customer client, I was thinking of just allowing a PSR-18 compliant client to be passed into the constructor, rather than a guzzle client. We could that create a small PSR-18 adapter for guzzle, that the package would use if a custom client was not provided.

What are you thoughts?

[nicklog]

Hey, thanks for your reply

However, I'm a little concerned that this seems very complex for just allowing developers to use a custom client and cache.

It's not as complicated as it looks. I just added PHP7 typing, implemented an abstract class, added an interface, and implemented getter methods.

I have implemented the ability to use a custom PSR-6 cache library in #23. I think this is somewhat simpler.

Thats good but...

In regards to allowing a customer client, I was thinking of just allowing a PSR-18 compliant client to be passed into the constructor, rather than a guzzle client. We could that create a small PSR-18 adapter for guzzle, that the package would use if a custom client was not provided.

.. I've already done all that. I used PSR-6 and PSR-18 for the constructor as well as the getter methods and PSR-7/PSR-17 for request generation. The Guzzle-Client-Adapter based on PSR-18 and your own cache implementation based on PSR-6 will only be created if you don't pass your own in the constructor. The abstract class is for other developers to make their own customizations and implementations.

What are you thoughts?

Since the changes seem to be multi-layered, the PHP version will be raised and various PSR standards will be introduced, I would suggest to make version 3.0 out of it. The PHP Unit tests are all successful.

The public methods are the same. Only the signature of the constructor has changed slightly. Otherwise, all changes only affect the inner workings of the classes.

What do you say?

[DivineOmega]

Hi. Thanks for your detailed reply. Much appreciated.

I'll re-review your code, hopefully this weekend, and get back to you. 👍

Thanks again.

[nicklog]

I have to thank you ;)

[DivineOmega]

I've noticed in the passwordExposedByHash method, you have modified the caching system to cache the password exposed status rather than the API response body. It was deliberate decision to cache the response body here, so that passwords with the same 5 characters at the beginning of their SHA1 hash could use the same cache file.

Even without this benefit, the current approach is using a cacheKey that is based on the first 5 characters, meaning the cached status would not be accurate for any other passwords that shared those same first 5 characters (of the hash).

Would you be okay to tweak your code to adopt the existing caching system (the response body, rather than the password exposed status)?

Hope this makes sense.

[nicklog]

Hey,

I get what you mean. Changed it so that the status is stored to the hash in the cache and additionally hashed the cache key again.

I just want to avoid unnecessary overhead.
And so each status has its own cache lifetime.

Do you agree?

[DivineOmega]

Hi there,

I'm afraid I'd still prefer the API body response to be cached rather than the status. Although I understand that the way you've implemented this reduces the overhead of parsing the response into a status, it also means that you are creating a directory containing the SHA1 hashes of every password attempted.

I'd consider this to be something of a security risk, as it exposes the passwords in a far less secure manner than the hash formats most web applications will store their passwords (usually with bcrypt or similar). SHA1 password hashes, especially those without salts, are very easy to crack or lookup in rainbow table.

Although somewhat less optimised, this was a deliberate implementation decision for security.

Does this make sense?

[nicklog]

Hey,

okay :) At the moment I can't think of any other option.
Then let's do it the proven way.

Do you have any other comments?

[DivineOmega]

This looks great, thank you.

My only two additional suggestion, which are not critical, but would be nice to have are the following.

• Ability to customise the cache expiry time, in the same manner as the cache and client can be overridden.
• Unit tests that focus on the overriding of the cache and client.

However, if you could resolve the Travis CI and StyleCI issues, I will look at getting this merged. Thanks again.

[nicklog]

Take a look.
Additional i added some information in the composer.json. Please check it if you agree.

If you allow it, please open a new repository called "DivineOmega/password-exposed-bundle" and give me write permission. Would like to write a Symfony bundle.

[DivineOmega]

I'll get this merged in shortly. I might have to tweak a few things related to the tests before release, but other than that looks great.

Thank you for your work on this.

I've created a repo for the Symfony bundle and provided you with write access.

https://github.com/DivineOmega/symfony-password-exposed-bundle

[DivineOmega]

v3.0.0 now released. 🎉

https://github.com/DivineOmega/password_exposed/releases/tag/v3.0.0

[nicklog]

Yeah, cool :)
I like it. Thank you for your open mind.