Add email verification before activation

[faisalahammad]

What?

Add an email verification step before the Email two-factor provider can be activated, aligning the Email provider's activation flow with the TOTP provider.

Fixes #778

Why?

Previously, users could enable Email 2FA without confirming ownership of the email address, which posed a risk of account lockout if the email was incorrect or inaccessible. Requiring a successful code verification before the provider can be enabled closes that gap.

How?

• User Options UI: For unverified users, the "Email" provider section shows a "Verify your email address" button. Clicking it sends a verification code via REST, a new input field accepts the code, and on success the provider is enabled and the section updates to the standard enabled state. (user_options(), providers/js/email-admin.js)
• REST API: Added POST /two-factor/1.0/email (send/validate verification codes) and DELETE /two-factor/1.0/email (reset verification status) via register_rest_routes(), rest_setup_email(), and rest_delete_email(), with the same permission check used by other providers.
• Verification logic: Two_Factor_Email::is_available_for_user() returns true only if the user is verified (or already has the provider enabled), checked via the _two_factor_email_verified user meta (VERIFIED_META_KEY). No core changes are needed on top of current master: Two_Factor_Core::get_available_providers_for_user() already calls is_available_for_user() for the fallback provider, so this gate is respected there.
• Backwards compatibility: Users who already have the Email provider enabled are treated as "legacy verified" and continue working without re-verification.
• Data integrity: A pre_user_options_update() hook prevents enabling the Email provider via the standard profile form save unless the user is verified.
• Emails: generate_and_email_token() accepts an $action argument (login vs verification_setup) to send context-appropriate subject and body.

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): GLM
Used for: Initial implementation, unit/REST tests, and merge conflict resolution on the rebase. The final code and tests were reviewed, tested, and edited by me.

Testing Instructions

New flow (fresh setup):

1. Go to Users > Profile, scroll to Two-Factor Options, and confirm "Email" is not enabled.
2. Click "Verify your email address".
3. Check your email for the verification code.
4. Enter the code and click "Verify".
5. Expected: the section updates and the "Email" checkbox is now checked and enabled.

No regression (legacy user):

1. Log in as a user who already has Email 2FA enabled.
2. Go to Users > Profile.
3. Expected: the "Email" checkbox remains checked and functional, with no re-verification prompt.

No regression (login flow):

1. As a verified user with Email 2FA enabled, log out and log back in using an emailed login code.
2. Expected: the login email code flow works unchanged (the login action path in generate_and_email_token()).

Screenshots or screencast

Before	After
The Email provider could be enabled directly from the profile form without confirming the address.

Changelog Entry

Changed - The Email two-factor provider now requires email address verification before it can be enabled.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: faisalahammad <faisalahammad@git.wordpress.org>
Co-authored-by: georgestephanis <georgestephanis@git.wordpress.org>
Co-authored-by: masteradhoc <masteradhoc@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[georgestephanis]

I want to see this go in, but I think to avoid merge conflicts it'll need to pause until #814 goes in, or that will need to pause for this. Or this can just start doing the newer external include. Either way.

Like the idea, but there's some extra changes in the PR that I don't think need to be in this PR? I'm looking at the distignore, and I'm not sure why it's changing from protected to public for the constructor ... possibly totally reasonable, I'm just trying to be thorough.

[Copilot]

Pull request overview

Adds an email-verification step for the Email two-factor provider by introducing a “verified” user-meta flag, a REST-driven verification flow in the profile UI, and guardrails to prevent enabling Email 2FA unless verified (with legacy compatibility).

Changes:

• Add VERIFIED_META_KEY and gate is_available_for_user() on verification (while allowing legacy-enabled users).
• Introduce Email provider REST endpoints to send/verify codes and to deactivate/reset verification state.
• Expand unit tests for email contents, availability gating, and profile-save behavior; update .distignore.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
providers/class-two-factor-email.php	Adds verification meta key, REST endpoints, updated email content handling, UI changes, and profile-save enforcement.
tests/providers/class-two-factor-email.php	Adds/updates tests for verification-context emails, availability rules, and pre_user_options_update() behavior.
.distignore	Ignores two-factor.zip from distribution exports.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[masteradhoc]

Hey @faisalahammad
Thanks for your PR :) as we've just merged #814 would you mind seperating your code as well to the seperate files that the PR added?

[faisalahammad]

I have updated the PR to address all the feedback:

• Rebased on master: Aligned the branch with the recent PR Move inline JS to external script files #814 merge.
• Code Separation: Moved the newly added inline JS for the Email provider into providers/js/email-admin.js and enqueued it properly, maintaining consistency with the new architecture.
• Constructor Visibility: Reverted Two_Factor_Email::__construct() back to protected to preserve the singleton pattern.
• .distignore: Reverted the exclusion of two-factor.zip.
• PHPCS / Nonce: Added a phpcs:ignore for $_POST manipulation in pre_user_options_update(), as nonce validation is handled by core on the profile page.
• Translators Comments: Fixed the duplicate translater comment block during the rebase.
• Test State Leak: Fixed the leakage of $_SERVER['REMOTE_ADDR'] by restoring variables at the end of the test.
• REST API Tests: Added comprehensive REST API tests covering permissions, empty code, invalid code, successful verification, and deleting setup in tests/providers/class-two-factor-email-rest-api.php.

Ready for another review!

[masteradhoc]

@faisalahammad could you please:

• update your branch to get the latest changes from two-factor?
• remove your changes from readme.txt and readme.md
• fix the lint errors noted here and here?

this would make it a lot easier to review the PR properly :)

[faisalahammad]

Hi @masteradhoc,

I’ve updated the branch based on your feedback:

1. Rebased: The branch is now rebased on the latest upstream/master.
2. Reverted Readme/Package: I’ve removed all changes to readme.md, readme.txt, and package.json to keep this strictly focused on email verification.
3. Fixed PHPStan item: Added safety checks in class-two-factor-core.php so is_wp_error() catches $available_providers correctly before conducting diff operations.

Could you please check and let me know if everything good to merge?

[faisalahammad]

Hi @masteradhoc,

Thanks for the feedback! I've updated the PR with the following changes:

1. Rebased: The branch is now completely rebased onto the latest two-factor/master, dropping unrelated edits.
2. Reverted Readme/Package changes: I’ve removed all changes to readme.md, readme.txt, and package.json to keep this strictly focused on the email verification feature.
3. Fixed errors & Linting: Fixed the PHPStan error noted by reviewing the is_wp_error() checks in class-two-factor-core.php. Also reverted the package.json JS lint scope changes, so the pre-existing JS lint errors from upstream are no longer failing the CI build here.

Everything should be green now. Could you please re-review when you have a chance?

[masteradhoc]

early feedback. @nimesh-xecurify could you check the tests added in this PR if they are fine?

[faisalahammad]

Hi @masteradhoc,

Thanks for the thorough review! I've pushed a fix addressing all your feedback:

@ since tag fixes:

• enqueue_assets(): Changed @since 0.10.0 → @since 0.16.0
• register_rest_routes(): Added @since 0.16.0
• rest_setup_email(): Added @since 0.16.0
• rest_delete_email(): Added @since 0.16.0
• pre_user_options_update(): Added @since 0.16.0

CI test failures fixed:

• test_user_two_factor_rest_setup_email_valid_code: Replaced undefined is_provider_enabled_for_user() with in_array() check
• test_user_can_delete_email_verification: Fixed setup order (verified meta before enabling provider)
• test_admin_can_delete_email_for_others: Added missing enable_provider_for_user() call
• test_generate_and_email_token_login_context_correct_args: Updated assertions to match actual email body text
• test_other_sessions_destroyed_when_enabling_2fa: Added verified meta before enabling Email 2FA
• test_user_options (backup codes): Updated assertion to check wp_scripts() localized data

Could you please re-review when you get a chance?

[georgestephanis]

@faisalahammad What do you see the flow being on this if the user changes their email address? Should the user have to re-verify the new one through two-factor, or should it silently change the email it sends to to the new one (ignoring if the new email inbox has some spam filtering or deliverability problems)

[faisalahammad]

@georgestephanis Right now it silently follows the account email. The provider does not store the address that was verified. It reads the account email at send time. So when the user changes their account email, the token goes to the new address. The verified flag stays set.

WordPress core already confirms a new profile email before it applies the change. So the new address belongs to the user. But the token then goes to an address this provider never verified on its own. If that address has spam filtering or deliverability problems, the user can get locked out.

I see two options:

1. Keep the current behavior. The token follows the account email. Simple, and it always matches the account email.
2. Store the verified address in user meta. If the account email does not match it, the provider counts as not verified and the user must verify the new address. Stricter, but no lockout surprise.

I lean toward option 2 for safety. But I did not want to expand scope without your call. Which one do you prefer?

[faisalahammad]

Rebased the branch on the latest master and pushed the changes.

What changed since the last review:

• New @SInCE tags use 0.17.0.
• The email REST routes check the user ID and return 400 for an invalid user.
• The delete route disables the provider first, then deletes the verified meta. This matches the TOTP provider.
• is_available_for_user and pre_user_options_update now use get_enabled_providers_for_user for the legacy enabled check.
• The token email subject and message filters get a new action arg.
• user_options returns early for non WP_User values.
• The delete tests no longer enable the provider in setup. Master now needs session revalidation for that, and the tests match the TOTP tests now.
• login_html checks is_wp_error before it splits off the backup providers.

One behavior change to note: if a user has only removed providers enabled and Email is not verified for them, login now shows an error instead of the email fallback. It fails closed.

PHPCS, PHPStan and the full test suite pass locally.

[faisalahammad]

@masteradhoc @nimesh-xecurify The branch is rebased on the latest master and all feedback is addressed. The @SInCE tags now use 0.17.0 and the earlier CI test failures are fixed. Details in my review above. GitHub does not let me add you as reviewers, so pinging here. Could you please re-review when you have a chance?

[faisalahammad]

CI Fix Summary

Problem: The "Lint JS & CSS" check failed with 19 errors in Gruntfile.js. This PR did not touch that file. Dependabot bumped @wordpress/scripts to 35.0.0 on master. That version ships ESLint v10 with stricter rules.

Fix (55ae3ed):

• Remove the /* eslint-env */ comment. ESLint v10 no longer supports it. Node globals now come from the wp-scripts config.
• Reformat Gruntfile.js to match the current prettier rules.

Result:

Check	Before	After
Lint JS & CSS	Fail (19 errors)	Pass
Build	Pass	Pass

No plugin behavior changes. Build config only.

[faisalahammad]

I rebased this branch onto current master and there is one design question I need a maintainer decision on.

Rebase result

The branch is now 4 commits on top of master. The diff is 5 files:

• providers/class-two-factor-email.php
• providers/js/email-admin.js
• tests/providers/class-two-factor-email.php
• tests/providers/class-two-factor-email-rest-api.php
• tests/class-two-factor-core.php

Two things were dropped because master already fixed them better:

• The Gruntfile.js commit. master ships the new ESLint config format and the qrcode path fix (qrcode-generator/dist/qrcode.js plus a rename callback). Keeping this branch's version would undo that.
• The class-two-factor-core.php changes. master already handles is_wp_error at every caller, and get_available_providers_for_user() already calls is_available_for_user() on the fallback provider (line 791). So the file is now identical to master.

Tests pass (272 tests, 851 assertions) and all linters pass.

The question: fallback lockout

master documents the fallback provider like this (line 765):

The returned provider must be usable without any prior per-user setup (like the email provider is), since the user has no working provider left to configure it through.

This PR makes the Email provider require prior setup (verification). Combine the two and a specific user gets locked out:

1. A user has one provider enabled, for example TOTP, and never verified email.
2. An admin unticks that provider in Settings > Two-Factor, so the stored provider is no longer registered.
3. At login, get_available_providers_for_user() finds no registered stored provider, tries the Two_Factor_Email fallback, and is_available_for_user() now returns false because the user is not verified.
4. That hits the no_available_2fa_methods WP_Error path and wp_die(). The user cannot log in and cannot reach the profile page to verify. Only an admin or a DB change can recover them.

Before this PR the same user was emailed a code and could log in. The legacy exemption in is_available_for_user() cannot help here, because the enabled providers list is empty by the time the fallback runs.

Two ways to resolve it:

• A (my recommendation): keep the fail-closed behavior and update the master docblock to drop the "like the email provider is" example. This matches the intent of this PR and is the safer security posture. The tradeoff is that the locked-out user needs admin help.
• B: exempt the fallback from the verification gate, so a user whose providers were removed still gets emailed codes. This keeps master's documented contract, but reopens the lockout risk this PR is closing.

I did not decide this on my own because it changes auth behavior beyond the original review feedback. Please tell me which way to go. @masteradhoc @georgestephanis

The build zip and manual test steps are ready. I will keep this as a draft-level change until we settle the question above.