-
Notifications
You must be signed in to change notification settings - Fork 812
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 custom password strength meter #41485
Account Protection: Add custom password strength meter #41485
Conversation
…add/protect/account-protection-settings
…add/protect/account-protection-settings
…add/protect/account-protection-settings
projects/packages/account-protection/src/js/jetpack-password-strength-meter.js
Outdated
Show resolved
Hide resolved
projects/packages/account-protection/src/css/strength-meter.css
Outdated
Show resolved
Hide resolved
const coreValidationFailed = ! ( | ||
corePasswordStrengthResultsClass.includes( 'strong' ) || | ||
corePasswordStrengthResultsClass.includes( 'good' ) | ||
); |
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 can see how it can get complicated here surrounding accessing the state of the core password strength meter on the client side, maintaining that integration, etc.
It feels fragile to depend on the class name of a DOM element, but I understand there are probably limitations here.
I wonder if checking the current presence of the insecure password acknowledgement checkbox on the page would be any better? Edit: It does look like the short bad good strong
classes used by core is the best indicator we have.
Zooming out a bit as well, it would be great to give more context re: the "passes core validation" checkbox. If core does not provide any "why", perhaps we could have the password strength meter status mimic cores as a final fallback, and remove the "passes core validation" item entirely. WDYT?

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 thought about this one quite a bit and went and back and forth on it.
The original idea was to make every effort to not modify or remove anything that core does currently, and instead hook into the current validation process and add our custom conditions on top so we can continue to use other related core functionality (eg. auto population, showing/hiding the field) and keep the original form submission handling clean.
It's not ideal, but the only obvious way I found to verify the success of the core validation was to check the class. Checking for the presence of the acknowledgement could work, but then it might get more complicated when our own conditions trigger it to hide or show.
I'll note that the core validation does appear to run a number of unique checks so I do think keeping it could be valuable. Unfortunately, there is no explanation provided when the core validation fails.
If we leave the core validation handling intact but don't include it in our set, things external to core strength meter continue to happen (eg when core validation fails the insecure password acknowledgement checkbox displays, submit button is disabled, etc). It might seem awkward if all our checks pass but core doesn't and the validation is green but they are still prompted to check the bypass to be able to submit.
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 might seem awkward if all our checks pass but core doesn't and the validation is green but they are still prompted to check the bypass to be able to submit.
In the case where the core validation is the failing check, we could mirror the status in our status bar.
We wouldn't necessarily need to remove the checklist item, maybe it is just the "Passes core validation" label that I am worried about. It is a lot of jargon. WDYT of just labeling it "Strong password"?
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 that makes sense to me, the current label is pretty ambiguous. We could also only display it if its failing, similar to the backslashes item? Do you think we should modify the tooltip or is that reasonable as is?
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.
projects/packages/account-protection/src/js/jetpack-password-strength-meter.js
Outdated
Show resolved
Hide resolved
projects/packages/account-protection/src/js/jetpack-password-strength-meter.js
Outdated
Show resolved
Hide resolved
projects/packages/account-protection/src/css/strength-meter.css
Outdated
Show resolved
Hide resolved
projects/packages/account-protection/src/css/strength-meter.css
Outdated
Show resolved
Hide resolved
Aside - the core |
@nateweller Thanks for the thorough review - I've applied the requested changes. I've also documented some minor UI awkwardness to address in follow-up/clean-up tasks (if timing permits) here: https://github.com/Automattic/jetpack-scan-team/issues/1710 |
projects/packages/account-protection/src/class-validation-service.php
Outdated
Show resolved
Hide resolved
projects/packages/account-protection/src/class-validation-service.php
Outdated
Show resolved
Hide resolved
* Add Account Protection toggle to Jetpack security settings * Import package and run activation/deactivation on module toggle * changelog * Add Protect Settings page and hook up Account Protection toggle * changelog * Update changelog * Register modules on plugin activation * Ensure package is initialized on plugin activation * Make account protection class init static * Add auth hooks, redirect and a custom login action template * Reorg, add Password_Detection class * Remove user cxn req and banner * Do not enabled module by default * Add strict mode option and settings toggle * changelog * Add strict mode toggle * Add strict mode toggle and endpoints * Reorg and add kill switch and is supported check * Add testing infrastructure * Add email handlings, resend AJAX action, and attempt limitations * Add nonces, checks and template error handling * Use method over template to avoid lint errors * Improve render_password_detection_template, update SVG file ext * Remove template file and include * Prep for validation endpoints * Update classes to be dynamic * Add constructors * Reorg user meta methods * Add type declarations and hinting * Simplify method naming * Use dynamic classes * Update class dependencies * Fix copy * Revert unrelated changes * Revert unrelated changes * Fix method calls * Do not activate by default * Fix phan errors * Changelog * Update composer deps * Update lock files, add constructor method * Fix php warning * Update lock file * Changelog * Fix Password_Detection constructor * Changelog * More changelogs * Remove comments * Fix static analysis errors * Remove top level phpunit.xml.dist * Remove never return type * Revert tests dir changes in favour of a dedicated task * Add tests dir * Reapply default test infrastructure * Reorg and rename * Update @Package * Use never phpdoc return type as per static analysis error * Enable module by default * Enable module by default * Remove all reference to and functionality of strict mode * Remove unneeded strict mode code, update Protect settings UI * Updates/fixes * Fix import * Update placeholder content * Revert unrelated changes * Remove missed code * Update reset email to two factor auth email * Updates and improvements * Reorg * Optimizations and reorganizations * Hook up email service * Update error handling todos, fix weak password check * Test * Localize text content * Fix lint warnings/errors * Update todos * Add error handling, enforce input restrictions * Move main constants back entry file * Fix package version check * Optimize setting error transient * Add nonce check for resend email action * Fix spacing * Fix resend nonce handling * Email service fixes * Fixes, improvements to doc consistency * Add remaining password validation * Update weak password check returns * Fix phan errors * Revert prior change * Fix meta key * Add process for add/updating recent pass list * Send auth code via wpcom only * Update method name * Optimize validation * Fix key, remove testing code * Fix docs * Add foundation for the custom password strength meter * Fix tests * Add ajax request for password validation * Improve matches user data logic * Remove password reset nonce verification code * Updates and fixes * Updates and improvements * Include tests for new validation methods * Include tests for new validation methods * Add password manager class tests * Add password validation status handling and hook up ajax callback * Update variables names * Add loading state * Remove todos * Add nonce to ajax request * Remove custom nonce, add core create-user nonce check * Remove todos - always run server side validation * Update constant naming * Translate error message * Ensure styles are enqueued when viewing the password detection page * Use global page now and action check to enqueue styles * Skip recent password checks during create user action * Additional skips, and comment clarification * Revert skips of user specific reset form validation, hook provides access to this * Revert unintended additions * Return early if update is irrelevant * Only verify nonce if pass is set * Skip validation if bypass enabled * Improve logic * Improvements and reorg * Add info popovers * Add core req to initial validation state * Generalize core info popover message * Fix core strength meter status * Remove testing code * Ensure save enabled when appropriate * Update todos * Center validation items * Fix tests * Save alt approach * Fix styling, centralize core references * Reorg * Use global pagenow for context, restrict user specific check to profile updates * Compartmentalize generating and appending validation meter and status initial states * Optimization and reorg improvements * Remove todos * Remove unneeded comments * Ensure info popover fits in all form views * Fix test * Fix test * Update methods, removes nonce checks, fix tests * Fix test * Remove comment * Fix bindEvents * Correct colors * Add aria-live attr to strength-meter * Remove core input mods and use custom selectors to apply strength meter margins * Update core validation item message, and display only on failure * Add clarifying comment * Remove unnecessary user->ID check, and redundant method
Description
Replaces the core password strength meter UI—found in password management UIs (add new user, update profile, and password reset forms)—with a branded Jetpack strength meter that applies a custom set of password validation rules. We can enqueue our own JS to run on top of the core validation (using its set as an additional check) and our set to further harden to validation process without needing change the form submission flow or any major core functionality.
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
/wp-admin/user-new.php
/wp-admin/user-edit.php?user_id=<user_id>
/wp-admin/profile.php
/wp-login.php?action=lostpassword
Screenshots
profile.php
(user-specific):new-user.php
(non user-specific):edit-user.php
(non user-specific):action=rp || action=resetpass
(non user-specific):