Initial P0wnedPassword api checker #15

gnat42 · 2018-02-27T16:16:19Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT

PwnedPassword api checker. Fails validation if the user's password was found in the 500 million compromised password database.

sstok

Great work so far, I left some comments. If could take care of these, thanks 👍

sstok · 2018-02-28T10:21:10Z

src/Validator/Constraints/P0wnedPasswordValidator.php

+     */
+    public function validate($value, Constraint $constraint)
+    {
+        if (!empty($value)) {


Better to use an early return and check for a null values explicitly:

if (null === $password) { return; } if (!is_scalar($password) && !(is_object($password) && method_exists($password, '__toString'))) { throw new UnexpectedTypeException($password, 'string'); } $password = (string) $password;

FYI. By convention all Password validators use $password as value argument.

sstok · 2018-02-28T10:27:38Z

tests/Validator/P0wnedPasswordValidatorTest.php

+
+    public function testFound()
+    {
+        $builder = $this->createMock(ConstraintViolationBuilderInterface::class);


Please use the ConstraintValidatorTestCase provided by Symfony 👍
See https://github.com/rollerworks/PasswordStrengthValidator/blob/master/tests/Validator/PasswordStrengthTest.php for usage (you may also need the getMock() method; but I am not sure if this already fixed for older Symfony versions).

This makes testing easier and prevents testing against a mock ViolationsBuilder (which has actually caused some issues in the past).

sstok · 2018-02-28T10:28:14Z

src/Validator/Constraints/P0wnedPasswordValidator.php

+            if ($result->wasFound()) {
+                $this->context
+                    ->buildViolation($constraint->message)
+                    ->setParameter('{{ used }}', number_format($result->getUseCount()))


Don't format the number here as number_format uses the current system locale and not every region uses the same separator. This should be handled when rendering of the message.

how would it be handled when rendering?

sstok · 2018-02-28T10:33:24Z

composer.json

@@ -20,7 +20,10 @@
    ],
    "require": {
        "php": "^5.6 || ^7.0",
+        "guzzlehttp/psr7": "^1.4",


Please move these dependencies to require-dev as not everyone uses this functionality.
To ensure a compatible version is installed (of php-http/httplug add a conflict rule).

So I'm not sure if this makes sense as I understand it. This doesn't require guzzle, just the PSR implementation and if someone doesn't use it but then wants to they would have to install all -dev packages in production as well. Are you sure? We could either make it a suggest and exclude the validator if the package doesn't exist in a compiler pass, or implement our own PSR-7 Request object. Again, this package is small and only implements the PSR-7 interfaces.

OK, no problem then but should still be moved to require-dev to not force this anyone who wants to use this library but not this specific validator.

sstok · 2018-02-28T10:34:45Z

src/P0wnedPassword/Request/Client.php

+
+namespace Rollerworks\Component\PasswordStrength\P0wnedPassword\Request;
+
+use GuzzleHttp\Psr7\Request;


Is it possible to use the Request object of php-http/httplug? This would remove the requirement to use Guzzle, which is exactly why we have PSR-7 😄

I wanted to but php-http/httplug has no Request object. So the option is to create our own PSR-7 compatible Request object or to use a PSR7 implementing library.

Also this doesn't pull in guzzle, it only pulls in a PSR-7 implementation. See how small it is here: https://github.com/guzzle/psr7/tree/master/src

OK 👍 keep it.

sstok · 2018-02-28T10:35:44Z

src/P0wnedPassword/Request/Client.php

+    private $logger;
+
+    /**
+     * Client constructor.


No need for phpdoc in Constructor. The typehints are enough 👍

sstok · 2018-02-28T10:36:22Z

src/P0wnedPassword/Request/Client.php

+use Http\Client\HttpClient;
+use Psr\Log\LoggerInterface;
+
+class Client


Should be marked as @internal to prevent accidentally using this outside of this package.

what's wrong with using it outside the package? Suppose someone wants to check the password but not fail as it would through a validator?

Oh wait we can't make it internal because then you can't "use" it for service injection 😅
OK no problem then.

sstok · 2018-02-28T10:36:42Z

src/P0wnedPassword/Request/Result.php

+
+namespace Rollerworks\Component\PasswordStrength\P0wnedPassword\Request;
+
+class Result


@internal please 👍

Same question as above

sstok · 2018-03-08T11:35:36Z

Sorry for the late reply, busy week 😃

sstok · 2018-03-10T11:10:43Z

composer.json

@@ -20,14 +20,17 @@
    ],
    "require": {
        "php": "^5.6 || ^7.0",
+        "php-http/httplug": "^1.1",


This still needs to be moved to require-dev as not everyone uses this specific validator. And optionally to prevent getting unsupported versions a conflict rule, but this mainly depends on if php-http/httplug has a version lower then 1.0 which is still actively used today.

There is only a v0.1.0 then 1.0.x releases. I'm pretty sure httplug came about because of PSR7 and dependency issues with bundles requiring different versions of guzzle. So 1.0 was the first real version

OK, no need for a conflict rule then 👍

sstok · 2018-03-10T15:07:07Z

Thank you @gnat42

gnat42 · 2018-03-15T02:27:44Z

no problem!

stof · 2018-03-20T11:54:16Z

src/Validator/Constraints/P0wnedPassword.php

+
+class P0wnedPassword extends Constraint
+{
+    public $message = 'This password was found in a database of compromised passwords. It has been used {{ used }} times. For security purposes you must use something else.';


This message should be added in translation files, otherwise it won't ever get translation.

@gnat42 Can you open a pull request for this?

ickbinhier · 2018-05-02T14:33:52Z

hello,

do I have to do more than insert the following lines after @RollerworksPassword\Blacklist?

     * @RollerworksPassword\P0wnedPassword(
     *     message="dein Passwort wurde in einer Datenbank kompromittierter Passwörter gefunden. Es wurde {{used}} mal verwendet. Aus Sicherheitsgründen müssen Sie etwas anderes verwenden."
     * )

The result is the following message.

is that a bug or my stupidity?

Type error: Too few arguments to function Rollerworks\Component\PasswordStrength\Validator\Constraints\P0wnedPasswordValidator::__construct(), 0 passed in /var/www/html/vendor/symfony/symfony/src/Symfony/Component/Validator/ContainerConstraintValidatorFactory.php on line 52 and exactly 1 expected

gnat42 · 2018-05-02T15:08:07Z

Hmm... I don't 100% remember the code anymore - however the docs state you need a client. I don't know if we need to do this but I also manually configure the two clients in my bundle services for the validator.

  Rollerworks\Component\PasswordStrength\P0wnedPassword\Request\Client:
    arguments: [ '@httplug.client.powned_checker','@logger']

  Rollerworks\Component\PasswordStrength\Validator\Constraints\P0wnedPasswordValidator:
    arguments: ['@Rollerworks\Component\PasswordStrength\P0wnedPassword\Request\Client']
    tags:
      - { name: validator.constraint_validator }

Probably need to flesh out the documentation here... I think they didn't want a guzzle/http-plug client implementation required - so you need to configure require and configure one, then configure the validator to use it like I do above. In a symfony bundle we will have to improve it so that its a matter of configuration for that bundle but at the moment if you are using symfony you need to configure the validator service by providing a client

sstok · 2018-08-05T15:13:44Z

Feel free to open a pull request in the Bundle repository to add support this, I guess the https://github.com/php-http/HttplugBundle would be the best option to use the Client. Optional dependency of course ;)

gnat42 force-pushed the pwnedpassword-api branch 4 times, most recently from 5b60a35 to e01e853 Compare February 27, 2018 16:34

sstok added the Feature label Feb 28, 2018

sstok requested changes Feb 28, 2018

View reviewed changes

gnat42 force-pushed the pwnedpassword-api branch from e01e853 to 94e3d23 Compare February 28, 2018 15:48

gnat42 force-pushed the pwnedpassword-api branch from 94e3d23 to eb3ad15 Compare March 8, 2018 16:56

sstok approved these changes Mar 10, 2018

View reviewed changes

sstok requested changes Mar 10, 2018

View reviewed changes

initial P0wnedPassword api checker

326b004

gnat42 force-pushed the pwnedpassword-api branch from eb3ad15 to 326b004 Compare March 10, 2018 14:28

sstok approved these changes Mar 10, 2018

View reviewed changes

Fix style

3236291

sstok merged commit 6943d80 into rollerworks:master Mar 10, 2018

stof reviewed Mar 20, 2018

View reviewed changes

gnat42 deleted the pwnedpassword-api branch August 24, 2018 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial P0wnedPassword api checker #15

Initial P0wnedPassword api checker #15

gnat42 commented Feb 27, 2018

sstok left a comment

sstok Feb 28, 2018

sstok Feb 28, 2018

sstok Feb 28, 2018

gnat42 Feb 28, 2018

sstok Feb 28, 2018

gnat42 Feb 28, 2018

sstok Mar 8, 2018

sstok Feb 28, 2018

gnat42 Feb 28, 2018

sstok Mar 8, 2018

sstok Feb 28, 2018

sstok Feb 28, 2018

gnat42 Feb 28, 2018

sstok Mar 8, 2018

sstok Feb 28, 2018

gnat42 Feb 28, 2018

sstok commented Mar 8, 2018

sstok Mar 10, 2018

gnat42 Mar 10, 2018

sstok Mar 10, 2018

sstok commented Mar 10, 2018

gnat42 commented Mar 15, 2018

stof Mar 20, 2018

sstok Mar 20, 2018

ickbinhier commented May 2, 2018

gnat42 commented May 2, 2018

sstok commented Aug 5, 2018


		namespace Rollerworks\Component\PasswordStrength\P0wnedPassword\Request;

		use GuzzleHttp\Psr7\Request;


		namespace Rollerworks\Component\PasswordStrength\P0wnedPassword\Request;

		class Result

Initial P0wnedPassword api checker #15

Initial P0wnedPassword api checker #15

Conversation

gnat42 commented Feb 27, 2018

sstok 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

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

sstok commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstok commented Mar 10, 2018

gnat42 commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ickbinhier commented May 2, 2018

gnat42 commented May 2, 2018

sstok commented Aug 5, 2018