Added password strength checker class and Sync listener. by zinigor · Pull Request #11643 · Automattic/jetpack

zinigor · 2019-03-21T17:09:49Z

This adds a class that listens to password verification events and checks the passwords for certain criteria.

Changes proposed in this Pull Request:

Adds a password checker class.
Adds a new Sync listener warning for insecure passwords.

Testing instructions:

Use a Jetpack site with this PR.
Sandbox your connection and apply D25323-code to your sandbox.
Set an insecure password for your user, for example, the same as your login.
Observe an audit log entry and a sticker in the MC.

Proposed changelog entry for your changes:

Added a password checker class.

As an accompanying code, also added the password strength checker library.

jetpackbot · 2019-03-21T17:15:21Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 2, 2019.
Scheduled code freeze: March 26, 2019

Generated by 🚫 dangerJS against 23e476b

enejb · 2019-03-21T18:44:13Z

sync/class.jetpack-sync-module-auth.php

+			return $user;
+		}
+
+		call_user_func(


Instead of doing this.
I think it would be cleared to do a

do_action( 'jetpack_password_check_failed', $user->ID, $test_results['test_results']['failed'] );

and then in listeners we can then just call

add_action( 'jetpack_password_check_failed', $this->handler, 10, 2 );

enejb · 2019-03-21T18:47:25Z

tests/php/sync/test_class.jetpack-sync-module-auth.php

+ */
+
+class WP_Test_Jetpack_Sync_Module_Auth extends WP_Test_Jetpack_Sync_Base {
+


❤️ Thanks for adding the tests

enejb · 2019-03-21T18:51:18Z

I am not 100% since I haven't tested this but it looks like authenticate filter is called on every login as well. Which is not some that we want.

I think it would be really interesting to always send an event when the user password changes.
and if they change the password to something that is not secure also send that as extra data with the error that we do want.

kraftbj · 2019-03-21T19:05:42Z

I am not 100% since I haven't tested this but it looks like authenticate filter is called on every login as well. Which is not some that we want.

At least use wp_authenticate_user instead?

I think it would be really interesting to always send an event when the user password changes.
and if they change the password to something that is not secure also send that as extra data with the error that we do want.

Of interest:

/**
	 * Fires before the password and confirm password fields are checked for congruity.
	 *
	 * @since 1.5.1
	 *
	 * @param string $user_login The username.
	 * @param string $pass1     The password (passed by reference).
	 * @param string $pass2     The confirmed password (passed by reference).
	 */
	do_action_ref_array( 'check_passwords', array( $user->user_login, &$pass1, &$pass2 ) );

I think that may be the only place we can access a changing password before it is hashed when an user is ~~inserted~~/updated.

kraftbj · 2019-03-21T20:41:18Z

_inc/lib/class.jetpack-password-checker.php

@@ -0,0 +1,1288 @@
+<?php //phpcs:ignore WordPress.Files.FileName.InvalidClassFileName


FYI @zinigor I added these ignore statements to the new files and added them to the phpcs whitelist since they're new and clean.

kraftbj · 2019-03-21T20:42:46Z

_inc/lib/class.jetpack-password-checker.php

+		} else {
+			$this->user_id = $user;
+		}
+		$this->min_password_length = apply_filters( 'better_password_min_length', $this->min_password_length );


Needs docblock and likely changed hook name to jetpack_password_checker, etc

kraftbj · 2019-03-21T20:45:12Z

_inc/lib/class.jetpack-password-checker.php

+		}
+
+		/**
+		 * Filters Jetpack's password strength enforcement settings. You can modify the minimum


We could include in the docs that 28 is weak, 32 is suggested, etc?

sync/class.jetpack-sync-module-auth.php

…/jetpack into add/password-strength-checker

matticbot · 2019-03-25T22:57:18Z

Caution: This PR has changes that must be merged to WordPress.com
Hello zinigor! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D25993-code before merging this PR. Thank you!

Only call clear_flags when we after we acually use the flags. We had an issue where we were cleading the flags after login in before we could acually use the flags.

matticbot · 2019-03-26T00:33:09Z

zinigor, Your synced wpcom patch D25993-code has been updated.

enejb · 2019-03-26T01:00:52Z

I tested this and it works as expected for me.

When I login as a user on my jetpack site as that has a week password. I can see the event come though on the .com side.

the code on .com will need have this PR applied to it.
D25997-code

matticbot · 2019-03-26T01:03:46Z

zinigor, Your synced wpcom patch D25993-code has been updated.

matticbot · 2019-03-26T14:26:46Z

zinigor, Your synced wpcom patch D25993-code has been updated.

* Initial Changelog for 7.2 * Testing list: add mention of IE11 testing * Initial Changelog for 7.2 * Testing list: add mention of IE11 testing * Add CL for #11224 * Add CL for #11426 * Add CL for #11442 * Add testing instructions for #11224 * Add CL for #11451 * Reclassify CL item * Add testing instructions for #11451 * Add CL for #11486 * Add CL for #11418 * Add CL for #11524 * Add CL and testing instructions for #11449 * Add CL for #11460 * Add CL for #11520 and #11582 * Add CL for #11531 * Add CL #11644 * Add testing instructions for #11644 * Add testing instructions for #11644 * Add CL for #11618 * Uniform changelog lines * CL #11679 * CL #11661 * CL #11654 * CL #11645 * CL #11643 * CL #11636 * CL #11635 and for other PHPCS commits * CL #11627 * CL #11626 * CL #11598 * CL #11596 * Remove nested items for shortcopy. I don't believe the detailed list is helpful * CL #11570 * CL #11569 * CL #11560 * CL #11558 * CL #11555 * CL #6704 * CL #11298 * CL #11324 * CL #11443 * CL #11484 * CL #11516 * CL #11529 * Expand Ads block enhancement CL item

zinigor added 2 commits March 21, 2019 18:27

Added the password strength checker sync listener.

1d6ee28

As an accompanying code, also added the password strength checker library.

Added a test for secure passwords.

6d38949

zinigor added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review This PR is ready for review. [Package] Sync labels Mar 21, 2019

zinigor added this to the 7.2 milestone Mar 21, 2019

zinigor self-assigned this Mar 21, 2019

zinigor requested review from a team, dereksmart and jhnstn March 21, 2019 17:09

enejb reviewed Mar 21, 2019

View reviewed changes

kraftbj added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Mar 21, 2019

Add files to phpcs whitelist

e830849

kraftbj requested changes Mar 21, 2019

View reviewed changes

zinigor added 2 commits March 22, 2019 19:24

Used a compatible assertion to avoid fatals in Travis.

7393a8d

Merge branch 'add/password-strength-checker' of github.com:Automattic…

86f3870

…/jetpack into add/password-strength-checker

kraftbj added [Status] In Progress and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 22, 2019

zinigor added 2 commits March 25, 2019 14:11

Fixed warnings to go ahead with the tests.

70f27e4

Moved the password checking into the User listener.

f0b1afc

matticbot added the Touches WP.com Files label Mar 25, 2019

enejb added 2 commits March 25, 2019 16:06

Update the jetpack_wp_login event to send IP Address.

c0222a4

Remove the additional clear_flags calls.

49beefc

Only call clear_flags when we after we acually use the flags. We had an issue where we were cleading the flags after login in before we could acually use the flags.

Remove the debug call

e1732cd

kraftbj added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Mar 26, 2019

Whitespace cleanup.

23e476b

kraftbj approved these changes Mar 26, 2019

View reviewed changes

kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 26, 2019

kraftbj merged commit 0088971 into master Mar 26, 2019

kraftbj deleted the add/password-strength-checker branch March 26, 2019 14:39

matticbot added the [Status] Needs Changelog label Mar 26, 2019

ghost removed [Status] Needs Changelog [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 26, 2019

kraftbj added [Status] Needs Changelog and removed [Status] Needs Changelog labels Mar 26, 2019

kraftbj added a commit that referenced this pull request Mar 26, 2019

CL #11643

4d33a04

		*/

		class WP_Test_Jetpack_Sync_Module_Auth extends WP_Test_Jetpack_Sync_Base {

		@@ -0,0 +1,1288 @@
		<?php //phpcs:ignore WordPress.Files.FileName.InvalidClassFileName

Conversation

zinigor commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

jetpackbot commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enejb Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

enejb Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enejb commented Mar 21, 2019

Uh oh!

kraftbj commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kraftbj Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

kraftbj Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

kraftbj Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matticbot commented Mar 25, 2019

Uh oh!

matticbot commented Mar 26, 2019

Uh oh!

enejb commented Mar 26, 2019

Uh oh!

matticbot commented Mar 26, 2019

Uh oh!

matticbot commented Mar 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zinigor commented Mar 21, 2019 •

edited

Loading

jetpackbot commented Mar 21, 2019 •

edited

Loading

enejb Mar 21, 2019 •

edited

Loading

kraftbj commented Mar 21, 2019 •

edited

Loading