Skip to content
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: Update password detection flow #41365

Conversation

dkmyta
Copy link
Contributor

@dkmyta dkmyta commented Jan 28, 2025

Description

Updates the password detection flow to accomodate change to scope

TODOs:

Proposed changes:

  • Hooks up the email endpoint
  • Hooks up the password validation endpoint
  • Updates the detection flow to send auth codes
  • Optimization and organization improvements

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

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

  • No

Testing instructions:

  • Checkout this branch
  • Run Jurassic Tube
  • Install and activate either Jetpack or Protect and enable the Account Protection settings
  • Log out and proceed to the wp-login.php form an attempt to log in
  • Ensure that after the validation fails you are redirected to wp-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 options
  • Ensure that the Resend email action sends an additional email, and enforces a limit of 3 attempts
  • Ensure an email is received and that the contents are as expected (via wp_mail() and WPcom server)
  • Verify that supplying the correct auth code logs you in and you are appropriately redirected
  • Ensure no regressions are introduced to core functionality and that the functionality only applies when the toggle is enabled
  • Review the package code responsible for each task

Screenshot

Screen Capture on 2025-01-28 at 12-19-28 (1)

dkmyta added 30 commits January 9, 2025 10:10
@dkmyta dkmyta mentioned this pull request Jan 29, 2025
3 tasks
Base automatically changed from remove/jetpack/account-protection-strict-mode to add/account-protection January 29, 2025 16:29
Copy link
Member

@ArSn ArSn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the blog URL in the email, I think most of these points are just some small stuff suggestions. Nice work!

@dkmyta dkmyta marked this pull request as ready for review January 29, 2025 19:04
@dkmyta dkmyta requested a review from ArSn January 29, 2025 19:04
@dkmyta
Copy link
Contributor Author

dkmyta commented Jan 29, 2025

@ArSn Regarding this comment, I made an attempt to correct this only to recall why I set it that way originally - the static analysis check returns an error when I use a void return or hint. I couldn't find a proper combination to resolve that other than using never.

Copy link
Member

@ArSn ArSn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well. 👍🏻

Two things of note:

  • There is no "visual confirmation" when resending the 2FA mail, this may be confusing to users (showing a message that it was resent when it was might be wise)
  • I don't think we're recommending the user to update the password anywhere in this flow right now (not sure if we have it planned in a different PR?)

Both are not blocking for this PR though.

@dkmyta dkmyta merged commit 98c2064 into add/account-protection Jan 30, 2025
57 of 59 checks passed
@dkmyta dkmyta deleted the update/packages/account-protection-password-detection-flow branch January 30, 2025 18:01
@github-actions github-actions bot removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] In Progress labels Jan 30, 2025
@jeherve jeherve added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Feb 5, 2025
nateweller pushed a commit that referenced this pull request Feb 20, 2025
* 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

* Fix phan errors

* Revert prior change

* Send auth code via wpcom only

* Update method name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu [Feature] WPCOM API [JS Package] API [Package] Account Protection [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] Protect A plugin with features to protect a site: brute force protection, security scanning, and a WAF. RNA [Tests] Includes Tests [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants