-
Notifications
You must be signed in to change notification settings - Fork 808
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Account Protection: Add password detection flow #41105
Account Protection: Add password detection flow #41105
Conversation
…add/protect/account-protection-settings
…add/protect/account-protection-settings
…add/protect/account-protection-settings
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
…tion-security-settings
…add/protect/account-protection-settings
…ges/account-protection-password-detection-flow
…add/protect/account-protection-settings
…ges/account-protection-password-detection-flow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small comments / notes, but nothing that keeps us from merging this. I was also able to test this, contrary to that other PR (which I will try again now).
* @return bool True if the password is valid, false otherwise. | ||
*/ | ||
public function validate_password( string $password ): bool { | ||
// TODO: Uncomment out once endpoint is live |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is actually live by now :)
|
||
// Check if the password is in the list of common/compromised passwords | ||
$password_suffix = substr( $hashed_password, 5 ); | ||
if ( in_array( $password_suffix, $body['compromised'] ?? array(), true ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might also want to check the common
index that is available now. However if we're just going to return true in both cases, we could also combine them. The idea was to adjust the messaging based on what hits, I think (tell the user specifically if it is a common or compromised one) - that being said, most common passwords are also compromised by their nature
<head> | ||
<meta charset="UTF-8"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
<title><?php echo esc_html( $reset ? 'Jetpack - Stay Secure' : 'Jetpack - Secure Your Account' ); ?></title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should all these texts be translateable? i.e. using esc_html__
functions et. al.?
<button class="action action-reset" type="submit" name="reset-password">Create a new password</button> | ||
</form> | ||
<form method="post"> | ||
<?php wp_nonce_field( 'proceed_action', '_wpnonce_proceed' ); ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we really need a nonce in this case, because we're not updating any data?
Significance: patch | ||
Type: changed | ||
|
||
Fixes Account Protection endpoint callback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR does a bit more than that? :)
…tion-security-settings
…count-protection-mutation.ts Co-authored-by: Kolja Zuelsdorf <[email protected]>
…ges/account-protection-password-detection-flow
…ges/account-protection-password-detection-flow
Merging as is, and will add follow-ups to address the items highlighted. |
Description
Add the password detection flow as described in the designs.
TODOs:
Reliant on:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
wp-login.php
form an attempt to log inwp-login.php?action=password-detection
and that the initial state of the page is styled as per designs and you are presented relevant details and optionsProceed without updating
button redirects you to/wp-admin
and that you now logged inCreate new password
button updates the content accordingly and that the email action is triggered only once on the initial render (disabled by setting and a checking a transient)Resend email
action dynamically updates the UI with status details and that the email function is triggeredScreen capture