Account Protection: Add password validation

[dkmyta]

Description

Adds Validation_Service methods for the following processes and accompanying tests.

• Must not be empty. (Profile forms only)
• Must not contain a backslash character. (Reset form only)
• Must be between 6 and 150 characters.
• Must not be present in a hardcoded list of common passwords.
• Must not be in the compromised passwords database (mirrored from Have I Been Pwned).
• Must not match user data (display name, nice name, first name, last name, email address, various parts of email address, nickname).
• Must not be a password that was already used by this user recently.

Other information:

• Have you written new tests for your changes, if applicable?
• Have you checked the E2E test CI results, and verified that your changes do not break them?
• Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

• https://github.com/Automattic/jetpack-scan-team/issues/1648
• https://github.com/Automattic/jetpack-scan-team/issues/1668

Does this pull request change what data or activity we track or use?

• No

Testing instructions:

• Checkout this branch and start up JT
• Login and enable Protect and ensure Account Protection is active
• Add a new user, update a current user and request/process a password reset
• Attempt to set your password to account for each of validation methods, ensure that the server side validation renders a error message and the password is not update
• Validate that we are correctly checking the current pass and that we are adding the old password to usermeta on each successful change
• Ensure no regressions are introduced
• Run cd projects/packages/account-protection && composer phpunit and verify that all tests pass

Screenshots

Reset form:

Profile form (add-new/update):

[nateweller]

Error thrown when creating a new account via wp-admin/user-new.php:

Uncaught Error: Automattic\Jetpack\Account_Protection\Validation_Service::is_recent_password(): Argument #1 ($user_id) must be of type int, null given, called in /usr/local/src/jetpack-monorepo/projects/packages/account-protection/src/class-validation-service.php on line 116
in /usr/local/src/jetpack-monorepo/projects/packages/account-protection/src/class-validation-service.php on line 252

Call stack:

    Automattic\J\A\Validation_Service::is_recent_password()
    /usr/local/src/jetpack-monorepo/projects/packages/account-protection/src/class-validation-service.php:116
    Automattic\J\A\Validation_Service::return_first_validation_error()
    /usr/local/src/jetpack-monorepo/projects/packages/account-protection/src/class-password-manager.php:89
    Automattic\J\A\Password_Manager::validate_profile_update()
    wp-includes/class-wp-hook.php:324
    WP_Hook::apply_filters()
    wp-includes/class-wp-hook.php:348
    WP_Hook::do_action()
    wp-includes/plugin.php:565
    do_action_ref_array()
    wp-admin/includes/user.php:226
    edit_user()
    wp-admin/user-new.php:198

[dkmyta]

Error thrown when creating a new account via wp-admin/user-new.php:

Right, good catch - its annoying but its looking like each form is going to need its own variation in checks. As it stands it seems like there might not be a way to perform user specific check on the reset form because the user is logged out, we also shouldn't run current/recent password checks when adding a new user since its not relevant there and the user reference wont have an ID because its not been generated yet. I'll pull this back out and see if I can get a unique path for each sorted out.

UPDATE: Tweaked the primary validation method to also skip the recent password check when were in create user context. I've already applied some conditional skips for the other forms and some comments for clarification.

[nateweller]

Suggested change

	/**
	* Return all validation errors.
	*
	* @param \WP_User|\stdClass $user The user object or a copy.
	* @param string $password The password to check.
	*
	* @return array An array of validation errors (if any).
	*/
	public function return_all_validation_errors( $user, string $password ): array {
	$errors = array();
	if ( $this->contains_backslash( $password ) ) {
	$errors[] = __( 'Doesn\'t contain a backslash (\\) character', 'jetpack-account-protection' );
	}
	if ( $this->is_invalid_length( $password ) ) {
	$errors[] = __( 'Between 6 and 150 characters', 'jetpack-account-protection' );
	}
	if ( $this->matches_user_data( $user, $password ) ) {
	$errors[] = __( 'Doesn\'t match user data', 'jetpack-account-protection' );
	}
	if ( $this->is_recent_password( $user->ID, $password ) ) {
	$errors[] = __( 'Not used recently', 'jetpack-account-protection' );
	}
	if ( $this->is_weak_password( $password ) ) {
	$errors[] = __( 'Not a leaked password.', 'jetpack-account-protection' );
	}
	return $errors;
	}

This return_all_validation_errors method doesn't appear to be used anywhere. Can it be removed?

[dkmyta]

It was added in preparation for the follow up where it will be used.

[nateweller]

My thought is that we shouldn't need to verify the core nonces inside hooks like user_profile_update_errors, profile_update, etc as these are already validated by core prior to calling these hooks.

I'm assuming these were added potentially in response to PHPCS issues when working with $_POST variables. There are a couple spots where we can use data provided directly from the hook, instead of the request data, and the remaining instances I'm thinking we can use phpscs:ignore for now.

Example Diff and Example Diff

[nateweller]

FYI phpcs:disable will disable PHPCS until phpcs:enable is used - we can use phpcs:ignore instead for single-line exclusions.

Example Diff

[nateweller]

Nit - unnecessary phpcs disable of unused param: Diff

[nateweller]

I have drafted a branch here which makes a few adjustments to:

• enable the use of a consistent $user type in methods
• adds a generic validate_user_password method that can be used anywhere, with duplicate errors from core removed and etc.

I would approve this PR with nonces removed, though I would like to circle back on items in the above at some point. Feel free to take a look and cherry-pick anything worthwhile.

[nateweller]

LGTM! Thanks for making these changes. Manually re-tested the add, edit, and password reset flows.