Merge pull request 'Validate signup email and password strength' (#155) from feature/150-signup-credential-validation into main

Reviewed-on: #155

tests/Unit/Auth/PasswordPolicyTest.php

@@ -0,0 +1,118 @@
+<?php
+declare(strict_types=1);
+
+namespace Unsupervised\Schedular\Tests\Unit\Auth;
+
+use Unsupervised\Schedular\Auth\PasswordPolicy;
+use Unsupervised\Schedular\Tests\Unit\TestCase;
+
+class PasswordPolicyTest extends TestCase
+{
+    public function testAcceptsAnOrdinaryMemorablePassword(): void
+    {
+        self::assertNull(PasswordPolicy::validate('thistle-marrow-42', '[email protected]', 'Grace Hopper'));
+    }
+
+    /**
+     * A leading or trailing space is a character like any other. Trimming it
+     * would accept a password the user could then never type back.
+     */
+    public function testCountsSurroundingSpaceAsPartOfThePassword(): void
+    {
+        self::assertNull(PasswordPolicy::validate('  spaced-out-phrase  '));
+
+        // Seven characters counting both spaces: one short, and still one short
+        // after the spaces are counted rather than stripped.
+        self::assertNotNull(PasswordPolicy::validate(' short '));
+    }
+
+    /**
+     * @dataProvider tooShort
+     */
+    public function testRejectsAPasswordShorterThanTheMinimum(string $password): void
+    {
+        self::assertStringContainsString('at least', (string) PasswordPolicy::validate($password));
+    }
+
+    /** @return array<string, array{string}> */
+    public static function tooShort(): array
+    {
+        return [
+            'empty'       => [''],
+            'one short'   => ['sevench'],
+            'a few chars' => ['abc'],
+        ];
+    }
+
+    /**
+     * @dataProvider commonPasswords
+     */
+    public function testRejectsAWellKnownPassword(string $password): void
+    {
+        self::assertStringContainsString('commonly used', (string) PasswordPolicy::validate($password));
+    }
+
+    /** @return array<string, array{string}> */
+    public static function commonPasswords(): array
+    {
+        return [
+            'password123'      => ['password123'],
+            'shouting'         => ['PASSWORD123'],
+            'mixed case'       => ['PassWord123'],
+            'a keyboard walk'  => ['qwertyuiop'],
+            'digits in a row'  => ['123456789'],
+            'the classic'      => ['iloveyou'],
+        ];
+    }
+
+    /**
+     * @dataProvider tooFewDistinctCharacters
+     */
+    public function testRejectsAPasswordBuiltFromAlmostNoDistinctCharacters(string $password): void
+    {
+        self::assertStringContainsString('repeated characters', (string) PasswordPolicy::validate($password));
+    }
+
+    /** @return array<string, array{string}> */
+    public static function tooFewDistinctCharacters(): array
+    {
+        return [
+            'one character'  => ['aaaaaaaaaa'],
+            'two alternating' => ['abababababab'],
+            'three'          => ['abcabcabcabc'],
+        ];
+    }
+
+    /**
+     * @dataProvider identityEchoes
+     */
+    public function testRejectsAPasswordContainingTheUsersOwnDetails(string $password, string $email, string $name): void
+    {
+        self::assertStringContainsString('name or email', (string) PasswordPolicy::validate($password, $email, $name));
+    }
+
+    /** @return array<string, array{string, string, string}> */
+    public static function identityEchoes(): array
+    {
+        return [
+            'the whole email'    => ['[email protected]!', '[email protected]', 'Grace'],
+            'the local part'     => ['grace-hopper-1906', '[email protected]', ''],
+            'the display name'   => ['xxhopperxx-2019', '[email protected]', 'Hopper'],
+            'differing in case'  => ['MyGRACEpassword', '[email protected]', ''],
+        ];
+    }
+
+    /**
+     * A two- or three-letter overlap with a name is coincidence, not a weakness,
+     * and refusing it would be baffling to the person typing.
+     */
+    public function testShortIdentityFragmentsDoNotTripTheCheck(): void
+    {
+        self::assertNull(PasswordPolicy::validate('bramble-thicket', '[email protected]', 'Bo'));
+    }
+
+    public function testAnEmptyIdentityIsNotTreatedAsContainedInEverything(): void
+    {
+        self::assertNull(PasswordPolicy::validate('bramble-thicket', '', ''));
+    }
+}

tests/Unit/Auth/RegistrationPageTest.php

@@ -37,6 +37,9 @@ class RegistrationPageTest extends TestCase
         Functions\when('sanitize_text_field')->alias(static fn ($v) => $v);
         Functions\when('sanitize_textarea_field')->alias(static fn ($v) => $v);
         Functions\when('sanitize_email')->alias(static fn ($v) => $v);
+        // Reached on every submit now that the email is validated before the
+        // password, so the password can be checked against it.
+        Functions\when('is_email')->alias(static fn (string $v): bool => (bool) preg_match('/^[^@\s]+@[^@\s]+\.[^@\s]+$/', $v));
         Functions\when('absint')->alias(static fn ($v) => (int) $v);
         // The birth-year check reads current_time('Y'), so answer that format
         // properly rather than leaving it to cast out of the datetime string.
@@ -45,6 +48,7 @@ class RegistrationPageTest extends TestCase
         );
         Functions\when('wp_enqueue_style')->justReturn(null);
         Functions\when('wp_enqueue_script')->justReturn(null);
+        Functions\when('wp_localize_script')->justReturn(true);
 
         $invites   = Mockery::mock(InviteRepository::class);
         $policies  = Mockery::mock(PolicyRepository::class);
@@ -114,7 +118,7 @@ class RegistrationPageTest extends TestCase
 
     public function testInviteBranchCreatesAndLogsInTheStudent(): void
     {
-        $_POST = [ 'password' => 'password123', 'display_name' => 'Ada' ];
+        $_POST = [ 'password' => 'thistle-marrow-42', 'display_name' => 'Ada' ];
 
         Functions\when('email_exists')->justReturn(false);
         Functions\when('wp_insert_user')->justReturn(42);
@@ -131,7 +135,7 @@ class RegistrationPageTest extends TestCase
 
     public function testInviteAcceptanceLinksClassGrantForTheEmail(): void
     {
-        $_POST = [ 'password' => 'password123', 'display_name' => 'Ada' ];
+        $_POST = [ 'password' => 'thistle-marrow-42', 'display_name' => 'Ada' ];
 
         Functions\when('email_exists')->justReturn(false);
         Functions\when('wp_insert_user')->justReturn(42);
@@ -151,7 +155,7 @@ class RegistrationPageTest extends TestCase
 
     public function testOpenBranchCreatesPendingWithoutLoginAndEmails(): void
     {
-        $_POST = [ 'password' => 'password123', 'display_name' => 'Ada', 'email' => '[email protected]' ];
+        $_POST = [ 'password' => 'thistle-marrow-42', 'display_name' => 'Ada', 'email' => '[email protected]' ];
 
         Functions\when('is_email')->justReturn(true);
         Functions\when('email_exists')->justReturn(false);
@@ -178,7 +182,7 @@ class RegistrationPageTest extends TestCase
 
     public function testGroupInviteCreatesPendingAutoApproveAccountEvenWhenClosed(): void
     {
-        $_POST = [ 'password' => 'password123', 'display_name' => 'Ada', 'email' => '[email protected]' ];
+        $_POST = [ 'password' => 'thistle-marrow-42', 'display_name' => 'Ada', 'email' => '[email protected]' ];
 
         Functions\when('is_email')->justReturn(true);
         Functions\when('email_exists')->justReturn(false);
@@ -346,7 +350,7 @@ class RegistrationPageTest extends TestCase
 
     public function testRejectsWhenARequiredPolicyIsUnaccepted(): void
     {
-        $_POST = [ 'password' => 'password123', 'display_name' => 'Ada', 'email' => '[email protected]' ];
+        $_POST = [ 'password' => 'thistle-marrow-42', 'display_name' => 'Ada', 'email' => '[email protected]' ];
 
         Functions\when('is_email')->justReturn(true);
 
@@ -365,7 +369,7 @@ class RegistrationPageTest extends TestCase
 
     public function testRejectsWhenARequiredAccountQuestionIsUnanswered(): void
     {
-        $_POST = [ 'password' => 'password123', 'display_name' => 'Ada', 'email' => '[email protected]' ];
+        $_POST = [ 'password' => 'thistle-marrow-42', 'display_name' => 'Ada', 'email' => '[email protected]' ];
 
         Functions\when('is_email')->justReturn(true);
 
@@ -385,7 +389,7 @@ class RegistrationPageTest extends TestCase
     public function testRecordsAccountAnswersOnSuccess(): void
     {
         $_POST = [
-            'password'     => 'password123',
+            'password'     => 'thistle-marrow-42',
             'display_name' => 'Ada',
             'us_answers'   => [ '5' => 'By a friend' ],
         ];
@@ -419,7 +423,7 @@ class RegistrationPageTest extends TestCase
 
     public function testMaybeHandleSubmitLogsInInviteAndRedirects(): void
     {
-        $_POST    = [ 'us_register' => '1', 'password' => 'password123', 'display_name' => 'Ada' ];
+        $_POST    = [ 'us_register' => '1', 'password' => 'thistle-marrow-42', 'display_name' => 'Ada' ];
         $_REQUEST = [ 'us_invite' => 'raw-token' ];
 
         Functions\when('is_user_logged_in')->justReturn(false);
@@ -618,7 +622,7 @@ class RegistrationPageTest extends TestCase
     public function testGuardianSignupCreatesEachChildAndRecordsTheirAnswers(): void
     {
         $_POST = [
-            'password'       => 'password123',
+            'password'       => 'thistle-marrow-42',
             'display_name'   => 'Grace',
             'us_is_guardian' => '1',
             'children'       => [
@@ -656,10 +660,61 @@ class RegistrationPageTest extends TestCase
         self::assertSame([[101, 'Piano'], [102, 'Violin']], $recorded);
     }
 
+    /**
+     * The browser gates on zxcvbn, but that is advice a client can decline to
+     * take. Nothing is created for a password the server refuses.
+     *
+     * @dataProvider refusedPasswords
+     */
+    public function testSignupRefusesAPasswordThePolicyRejects(string $password, string $expected): void
+    {
+        $_POST = [
+            'email'        => '[email protected]',
+            'password'     => $password,
+            'display_name' => 'Grace Hopper',
+        ];
+
+        Functions\when('email_exists')->justReturn(false);
+        Functions\expect('wp_insert_user')->never();
+
+        self::assertStringContainsString(
+            $expected,
+            $this->submit(new Invite(email: '[email protected]', token: 'hash'), false)
+        );
+    }
+
+    /** @return array<string, array{string, string}> */
+    public static function refusedPasswords(): array
+    {
+        return [
+            'too short'         => ['abc123', 'at least'],
+            'a known password'  => ['password123', 'commonly used'],
+            'barely any variety' => ['ababababab', 'repeated characters'],
+            'their own name'    => ['grace-hopper-1906', 'name or email'],
+        ];
+    }
+
+    public function testSignupRefusesAnAddressThatIsNotAnEmail(): void
+    {
+        $_POST = [
+            'email'        => 'not-an-email',
+            'password'     => 'thistle-marrow-42',
+            'display_name' => 'Grace',
+        ];
+
+        Functions\when('email_exists')->justReturn(false);
+        Functions\expect('wp_insert_user')->never();
+
+        self::assertStringContainsString(
+            'valid email address',
+            $this->submit(null, true)
+        );
+    }
+
     public function testGuardianSignupWithNoChildrenIsRejected(): void
     {
         $_POST = [
-            'password'       => 'password123',
+            'password'       => 'thistle-marrow-42',
             'display_name'   => 'Grace',
             'us_is_guardian' => '1',
             'children'       => [['name' => '', 'birth_year' => '', 'answers' => []]],
@@ -743,7 +798,7 @@ class RegistrationPageTest extends TestCase
     public function testGuardianSignupRejectsAChildMissingARequiredAnswer(): void
     {
         $_POST = [
-            'password'       => 'password123',
+            'password'       => 'thistle-marrow-42',
             'display_name'   => 'Grace',
             'us_is_guardian' => '1',
             'children'       => [
@@ -770,7 +825,7 @@ class RegistrationPageTest extends TestCase
     public function testAFailedChildRollsBackEveryUserCreatedIncludingTheGuardian(): void
     {
         $_POST = [
-            'password'       => 'password123',
+            'password'       => 'thistle-marrow-42',
             'display_name'   => 'Grace',
             'us_is_guardian' => '1',
             'children'       => [
@@ -807,7 +862,7 @@ class RegistrationPageTest extends TestCase
     public function testSignupPoliciesAreAcceptedPerChildAndAttributedToTheGuardian(): void
     {
         $_POST = [
-            'password'       => 'password123',
+            'password'       => 'thistle-marrow-42',
             'display_name'   => 'Grace',
             'us_is_guardian' => '1',
             'accept'         => [3],
@@ -846,7 +901,7 @@ class RegistrationPageTest extends TestCase
 
     public function testANonGuardianSignupIsUnchangedAndCreatesNoChildren(): void
     {
-        $_POST = ['password' => 'password123', 'display_name' => 'Ada'];
+        $_POST = ['password' => 'thistle-marrow-42', 'display_name' => 'Ada'];
 
         Functions\when('email_exists')->justReturn(false);
         Functions\when('wp_insert_user')->justReturn(42);