#259 Validation for breached passwords #264

DragonBe · 2019-04-13T21:22:13Z

This is the UndisclosedPassword validator discussed in #259 which uses PSR-18 http client interface for communicating with Have I Been Pwned? web service for validating passwords in a secure, safe way using K-Anonymity method.

IMPORTANT: The thing that might be a problem for this pull request is that PSR-18 composer package requires PHP 7.0 or higher.

All functionality is documented and provided with unit tests ensuring 100% code coverage.

Please review this pull request and let me know if I need to change or improve the functionality.

Ocramius · 2019-04-13T21:23:26Z

composer.json

@@ -30,7 +30,9 @@
        "zendframework/zend-math": "^2.6",
        "zendframework/zend-servicemanager": "^2.7.5 || ^3.0.3",
        "zendframework/zend-session": "^2.8",
-        "zendframework/zend-uri": "^2.5"
+        "zendframework/zend-uri": "^2.5",
+        "psr/http-client": "^1.0",


These should be near the other psr/* libs above

@Ocramius I can do that… but have you seen my request in regards PSR-18 not being compatible with PHP 5.6? ➡️ #259 (comment)

Yeah, I think we need to bump the dependency here: it is long overdue and should just be done.

Also, I'm using this occasion to re-express to @zendframework/community-review-team the fact that bumping PHP version can be freely done in minor releases.

I raised zendframework/maintainers#59 about this

Ocramius · 2019-04-13T21:24:39Z

src/UndisclosedPassword.php

+    const UNKNOWN_ERROR = 'unknownError';
+
+    protected $messageTemplates = [
+        self::PASSWORD_BREACHED => 'The provided password was used by others',


Reading this in the UI can be confusing for a user: who is "others" to them?

Ocramius · 2019-04-13T21:25:40Z

src/UndisclosedPassword.php

+
+    protected $messageTemplates = [
+        self::PASSWORD_BREACHED => 'The provided password was used by others',
+        self::WRONG_INPUT => 'The provided password failed to be correctly hashed, please verify your input',


Given usages of WRONG_INPUT, I'd say that:

the constant should be renamed to NOT_A_STRING (or something like that)

the message should reflect the same

Ocramius · 2019-04-13T21:26:12Z

src/UndisclosedPassword.php

+    protected $messageTemplates = [
+        self::PASSWORD_BREACHED => 'The provided password was used by others',
+        self::WRONG_INPUT => 'The provided password failed to be correctly hashed, please verify your input',
+        self::CONNECTION_FAILURE => 'Unable to reach HIBP service, please try again later',


HIBP is a text that doesn't make sense for the typical end-user: we'll need a better error message here too

Ocramius · 2019-04-13T21:26:45Z

src/UndisclosedPassword.php

+        self::PASSWORD_BREACHED => 'The provided password was used by others',
+        self::WRONG_INPUT => 'The provided password failed to be correctly hashed, please verify your input',
+        self::CONNECTION_FAILURE => 'Unable to reach HIBP service, please try again later',
+        self::UNKNOWN_ERROR => 'Something happened beyond our control, error reporting should give more details',


Also here: this error message is developer-friendly, but not end-user friendly

Ocramius · 2019-04-13T21:33:04Z

src/UndisclosedPassword.php

+            $this->error(self::UNKNOWN_ERROR);
+            return false;
+        }
+        if ($isPwnd) {


This variable is redundant - should be removed

Ocramius · 2019-04-13T21:33:17Z

src/UndisclosedPassword.php

+     */
+    private function getRangeHash($passwordHash)
+    {
+        $range = substr($passwordHash, self::HIBP_RANGE_BASE, self::HIBP_RANGE_LENGTH);


Variable redundant

Ocramius · 2019-04-13T21:34:06Z

src/UndisclosedPassword.php

+    const HIBP_API_TIMEOUT = 300;
+    const HIBP_CLIENT_UA = 'zend-validator';
+    const HIBP_CLIENT_ACCEPT = 'application/vnd.haveibeenpwned.v2+json';
+    const HIBP_RANGE_LENGTH = 5;


Looking at usages of the constants below, this makes kinda sense, but the naming of the constants themselves do not always make sense. If we can expand the names, that would be best.

Ocramius · 2019-04-13T21:34:44Z

src/UndisclosedPassword.php

+
+final class UndisclosedPassword extends AbstractValidator
+{
+    const HIBP_API_URI = 'https://api.pwnedpasswords.com';


@weierophinney all of these constants should be private in order to prevent needless BC breaks later on: can we please bump to ^7.1 before expanding the API surface beyond what should be public?

Ocramius · 2019-04-13T21:36:20Z

test/UndisclosedPasswordTest.php

+     *
+     * @param string $password
+     *
+     * @covers \Zend\Validator\UndisclosedPassword::__construct


Consider using a single @covers \Zend\Validator\UndisclosedPassword at class level: this sort of fine-grained coverage is more added maintenance work when someone splits a private detail into more private methods

@Ocramius

As suggested by @Ocramius in PR zendframework#264 I'm moving added PSR components to the existing PSR requirement in composer.json

Applying the following changes: - Improving error messages in the validator - Improving naming conventions for variable names and constants - Bubbling up important Exceptions to be taken care of higher up the stream - Removing unused or arbitrary variables, messages and constants - Simplifying coverage declarations in unit tests

@Ocramius

After reviewing the feedback provided by @Ocramius on PR zendframework#264 I've agreed with him that keeping the count of found passwords has no meaning in a validation context and have removed it from the code base.

froschdesign

First: 👍 for adding documentation.
Second: I added some small comments. The important part is the code example because we need a full working example.

froschdesign · 2019-04-15T08:51:40Z

doc/book/validators/undisclosed-password.md

+
+`Zend\Validator\UndisclosedPassword` allows you to validate if a given password was found in data breaches using the service [Have I Been Pwned?](https://www.haveibeenpwned.com), in a secure, anonymous way using [K-Anonymity](https://www.troyhunt.com/ive-just-launched-pwned-passwords-version-2) to ensure passwords are not send in full over the wire.
+
+> ## Installation requirements


For notes we uses h3 (###).

froschdesign · 2019-04-15T08:53:27Z

doc/book/validators/undisclosed-password.md

+
+## Basic usage
+
+To validate if a password was disclosed in a known data breach, you need to provide a HTTP Client that implements `psr\http\ClientInterface`, a `Psr\Http\Message\RequestFactoryInterface` and a `Psr\Http\Message\ResponseFactoryInterface` to the constructor and validate the password you want to check.


The namespace for ClientInterface is wrong. It must be Psr\Http\Client\ClientInterface.

froschdesign · 2019-04-15T09:02:14Z

doc/book/validators/undisclosed-password.md

+If the password was found via the service, `isValid` will return FALSE. If the password was not found, `isValid` will return TRUE.
+
+```php
+$validator = new Zend\Validator\UndisclosedPassword(


The example must be completely reproducible for the end user. At the moment, the example shows the same thing as the API, which is not very helpful. We need a full code example including a HTTP client and the factories.

I will update this piece of example code after I figure out which PSR-18 http client I could use in this example.

I was thinking about using the Guzzle6 adapter and factories, because zend-diactoros doesn't implement PSR-18 (yet), but I can go any direction.

@froschdesign In commit 1383af3 I have provided a working example of Zend\Validator\UndisclosedPassword using PSR7, PSR17 and PSR18 HTTP Client and Messages.

Please have a look at it if this is in line with your expectations. If not, please indicate how I can improve it.

froschdesign · 2019-04-15T09:04:38Z

doc/book/validators/undisclosed-password.md

+
+To validate if a password was disclosed in a known data breach, you need to provide a HTTP Client that implements `psr\http\ClientInterface`, a `Psr\Http\Message\RequestFactoryInterface` and a `Psr\Http\Message\ResponseFactoryInterface` to the constructor and validate the password you want to check.
+
+If the password was found via the service, `isValid` will return FALSE. If the password was not found, `isValid` will return TRUE.


Also set "false" and "true" in backticks and use lowercase letters. Thanks!

@froschdesign

I'm processing the easy to fix documentation issues pointed out by @froschdesign in PR zendframework#264

To make this validator PSR18 compliant we are implementing the `psr/http-client` interfaces.

We can now validate if a password has been seen in a password breach.

Running CodeSniffer indicated some issues

Mocking an interface implementing \Throwable was not possible, creating a dummy \Exception class implementing the interface is the easiest solution without complexity requiring further testing.

The input is verified at an earlier stage so no need for throwing an exception for a condition that never will be reached.

Adding coverage and more precise testing we can now achieve higher quality with less maintenance.

@azjezz

Instead of recreating a new request object, I'm using the request factory to build a new request object. This was indirectly suggested by @azjezz via Twitter (https://twitter.com/azjezz/status/1117112058032656384). One thing is for sure: I ❤️ the PHP community for they give me so much wisdom and make me a smarter person 🙂

@Ocramius

As suggested by @Ocramius in PR zendframework#264 I'm moving added PSR components to the existing PSR requirement in composer.json

Applying the following changes: - Improving error messages in the validator - Improving naming conventions for variable names and constants - Bubbling up important Exceptions to be taken care of higher up the stream - Removing unused or arbitrary variables, messages and constants - Simplifying coverage declarations in unit tests

@Ocramius

After reviewing the feedback provided by @Ocramius on PR zendframework#264 I've agreed with him that keeping the count of found passwords has no meaning in a validation context and have removed it from the code base.

No feature is complete if there's not some user documentation available, so with this I've provided a basic introduction on how to use the validator.

@froschdesign

I'm processing the easy to fix documentation issues pointed out by @froschdesign in PR zendframework#264

Since we have no knowledge how the client is configured, I'm using a fully qualified URI pointing to the HIBP API service. This will prevent issues making a connection.

To make it simple for users to make use of this validation feature, I created a working example people can use.

Adds license docblock to new class files. Makes constants in `UndisclosedPassword` private. Updates `UndisclosedPasswordTest` to use `ReflectionClass` when doing constant comparisons.

Bumps the minimum supported PHP version to 7.1, to allow us to use private constants (and reduce BC breaks going forwards). Adds support for PHP 7.4 to the test matrix.

And hopefully fix Travis test errors.

On all internal methods.

weierophinney · 2019-12-28T03:43:39Z

Thanks, @DragonBe! With the imminent transition to the Laminas Project, I am expecting we will likely revisit the ability to bump a PHP version during a major series, and, as such, decided to do so here, as it will ensure we do not have any BC issues with your new class in the future. I added visibility to all constants, and added typehints to parameters and return values of all internal methods. Will release shortly with version 2.13.0.

Ocramius suggested changes Apr 13, 2019

View reviewed changes

DragonBe added a commit to DragonBe/zend-validator that referenced this pull request Apr 13, 2019

Moving PSR components in composer.json

cad473a

As suggested by @Ocramius in PR zendframework#264 I'm moving added PSR components to the existing PSR requirement in composer.json

This was referenced Apr 13, 2019

Proposal: Validation for breached passwords #259

Open

Update in maintenance policy: allow for PHP version requirement changes in minor releases zendframework/maintainers#59

Open

DragonBe changed the base branch from master to develop April 13, 2019 22:15

froschdesign suggested changes Apr 15, 2019

View reviewed changes

DragonBe added a commit to DragonBe/zend-validator that referenced this pull request Apr 15, 2019

Processing minor feedback points in UndisclosedPassword documentation

528aaed

I'm processing the easy to fix documentation issues pointed out by @froschdesign in PR zendframework#264

michalbundyra requested review from froschdesign and Ocramius September 5, 2019 07:17

michalbundyra added this to the 2.13.0 milestone Sep 5, 2019

michalbundyra added the enhancement label Sep 5, 2019

michalbundyra changed the title ~~Enhancement/259~~ #259 Validation for breached passwords Sep 5, 2019

DragonBe and others added 16 commits December 27, 2019 20:59

Adding psr/http-client as suggested package

2ffb409

To make this validator PSR18 compliant we are implementing the `psr/http-client` interfaces.

Writing out the validator for undisclosed passwords

31d52d7

We can now validate if a password has been seen in a password breach.

Fixing coding style violations

24f0da0

Running CodeSniffer indicated some issues

Creating a HttpClientException for testing purposes

ccf548a

Mocking an interface implementing \Throwable was not possible, creating a dummy \Exception class implementing the interface is the easiest solution without complexity requiring further testing.

Removing unreached exceptions

a6e5fca

The input is verified at an earlier stage so no need for throwing an exception for a condition that never will be reached.

Improving tests

f779428

Adding coverage and more precise testing we can now achieve higher quality with less maintenance.

Moving PSR components in composer.json

4bd8861

As suggested by @Ocramius in PR zendframework#264 I'm moving added PSR components to the existing PSR requirement in composer.json

Removing password found count

7d63778

After reviewing the feedback provided by @Ocramius on PR zendframework#264 I've agreed with him that keeping the count of found passwords has no meaning in a validation context and have removed it from the code base.

Adding documentation for UndisclosedPassword validator

e3c334f

No feature is complete if there's not some user documentation available, so with this I've provided a basic introduction on how to use the validator.

Processing minor feedback points in UndisclosedPassword documentation

ce3eaa9

I'm processing the easy to fix documentation issues pointed out by @froschdesign in PR zendframework#264

Creating a full qualified URI

c709e51

Since we have no knowledge how the client is configured, I'm using a fully qualified URI pointing to the HIBP API service. This will prevent issues making a connection.

Providing a working example documentation

3095cd3

To make it simple for users to make use of this validation feature, I created a working example people can use.

fixup: move and edit docs

e4c1b55

qa: make constants private

425d608

Adds license docblock to new class files. Makes constants in `UndisclosedPassword` private. Updates `UndisclosedPasswordTest` to use `ReflectionClass` when doing constant comparisons.

weierophinney added 5 commits December 27, 2019 21:27

feat: bump PHP requirement to 7.1, and support 7.4

07e51da

Bumps the minimum supported PHP version to 7.1, to allow us to use private constants (and reduce BC breaks going forwards). Adds support for PHP 7.4 to the test matrix.

fix: runs update_hostname_validator to update hostname list

89a6646

And hopefully fix Travis test errors.

fix: add typehints to UndisclosedPassword

46393e9

On all internal methods.

qa: remove intermediate variable

66d9b3e

docs: adds CHANGELOG entry for zendframework#264

d3355e9

weierophinney force-pushed the enhancement/259 branch from 1383af3 to d3355e9 Compare December 28, 2019 03:38

weierophinney merged commit d3355e9 into zendframework:develop Dec 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#259 Validation for breached passwords #264

#259 Validation for breached passwords #264

DragonBe commented Apr 13, 2019

Ocramius Apr 13, 2019

DragonBe Apr 13, 2019

Ocramius Apr 13, 2019

Ocramius Apr 13, 2019

Ocramius Apr 13, 2019

Ocramius Apr 13, 2019

Ocramius Apr 13, 2019

Ocramius Apr 13, 2019

Ocramius Apr 13, 2019

Ocramius Apr 13, 2019

Ocramius Apr 13, 2019

Ocramius Apr 13, 2019

Ocramius Apr 13, 2019

froschdesign left a comment

froschdesign Apr 15, 2019

froschdesign Apr 15, 2019

froschdesign Apr 15, 2019

DragonBe Apr 15, 2019

DragonBe Apr 17, 2019

froschdesign Apr 15, 2019

weierophinney commented Dec 28, 2019


		`Zend\Validator\UndisclosedPassword` allows you to validate if a given password was found in data breaches using the service [Have I Been Pwned?](https://www.haveibeenpwned.com), in a secure, anonymous way using [K-Anonymity](https://www.troyhunt.com/ive-just-launched-pwned-passwords-version-2) to ensure passwords are not send in full over the wire.

		> ## Installation requirements


		## Basic usage

		To validate if a password was disclosed in a known data breach, you need to provide a HTTP Client that implements `psr\http\ClientInterface`, a `Psr\Http\Message\RequestFactoryInterface` and a `Psr\Http\Message\ResponseFactoryInterface` to the constructor and validate the password you want to check.


		To validate if a password was disclosed in a known data breach, you need to provide a HTTP Client that implements `psr\http\ClientInterface`, a `Psr\Http\Message\RequestFactoryInterface` and a `Psr\Http\Message\ResponseFactoryInterface` to the constructor and validate the password you want to check.

		If the password was found via the service, `isValid` will return FALSE. If the password was not found, `isValid` will return TRUE.

#259 Validation for breached passwords #264

#259 Validation for breached passwords #264

Conversation

DragonBe commented Apr 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

froschdesign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Dec 28, 2019