Skip to content

Commit 7abc9ca

Browse files
nimesh-xecurifyclaude
nimesh-xecurify
and
claude
committed
Add WP-CLI command tests and address review feedback
Adds PHPUnit coverage for Two_Factor_CLI_Command (status, disable, enable, list-providers, backup-codes generate, unlock) and for the new Two_Factor_Core::clear_login_rate_limit() helper. Because the WP-CLI runtime is not loaded during PHPUnit, lightweight test doubles for WP_CLI, WP_CLI_Command, and the WP_CLI\Utils helpers are added under tests/cli/ to capture output and simulate the error()/confirm() exit behaviour. Also fixes the issues raised in review on PR WordPress#905: - backup-codes generate now enables the Two_Factor_Backup_Codes provider (mirroring rest_generate_codes()) so the codes are usable at login instead of being stored but never offered. - enable, disable, and the full reset now destroy the user's sessions when their enabled providers change, matching the profile-page behaviour. - The full reset no longer clears _two_factor_password_was_reset, so a rescued user still sees the notice explaining why their old password stopped working. - Removes the misleading reference to a non-existent "Phase 3" totp subcommand from the TOTP enable error message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 6faf97b commit 7abc9ca

8 files changed

Lines changed: 1050 additions & 7 deletions

‎CLI/class-two-factor-cli-command.php‎

Lines changed: 59 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,28 @@ private function resolve_user( $identifier ) {
3535
return false;
3636
}
3737

38+
/**
39+
* Destroy all of a user's sessions when their enabled providers changed.
40+
*
41+
* Mirrors the session invalidation the profile-page path performs (see
42+
* Two_Factor_Core::user_two_factor_options_update()) when 2FA settings
43+
* change. A configuration change made out-of-band via the CLI should take
44+
* effect immediately and force any active session to re-authenticate.
45+
*
46+
* @param WP_User $user Target user.
47+
* @param array $providers_before Enabled provider keys captured before the change.
48+
*/
49+
private function destroy_sessions_if_providers_changed( $user, $providers_before ) {
50+
$providers_after = Two_Factor_Core::get_enabled_providers_for_user( $user );
51+
52+
sort( $providers_before );
53+
sort( $providers_after );
54+
55+
if ( $providers_before !== $providers_after ) {
56+
WP_Session_Tokens::get_instance( $user->ID )->destroy_all();
57+
}
58+
}
59+
3860
/**
3961
* Show two-factor authentication status for a user.
4062
*
@@ -109,9 +131,11 @@ public function status( $args, $assoc_args ) {
109131
* Disable two-factor authentication for a user.
110132
*
111133
* Without a provider argument every factor is disabled and the user is
112-
* returned to a clean, pre-2FA baseline (nonce, lockout timers, the
113-
* password-was-reset flag, and all provider secrets are also cleared). With
114-
* a provider argument only that single factor is removed and the others are
134+
* returned to a clean, pre-2FA baseline (nonce, lockout timers, and all
135+
* provider secrets are also cleared, and existing sessions are destroyed).
136+
* The compromised-password-reset flag is intentionally preserved so the
137+
* rescued user still sees why their old password stopped working. With a
138+
* provider argument only that single factor is removed and the others are
115139
* left intact.
116140
*
117141
* The command is idempotent: disabling an already-disabled user or provider
@@ -194,6 +218,8 @@ private function disable_single_provider( $user, $provider, $assoc_args ) {
194218
);
195219

196220
if ( Two_Factor_Core::disable_provider_for_user( $user->ID, $provider ) ) {
221+
$this->destroy_sessions_if_providers_changed( $user, $enabled );
222+
197223
WP_CLI::success(
198224
sprintf(
199225
/* translators: 1: provider class name, 2: user login */
@@ -256,11 +282,15 @@ private function disable_all_providers( $user, $assoc_args ) {
256282
delete_user_meta( $user->ID, Two_Factor_Core::ENABLED_PROVIDERS_USER_META_KEY );
257283
delete_user_meta( $user->ID, Two_Factor_Core::PROVIDER_USER_META_KEY );
258284

259-
// Clear session and throttle state.
285+
// Clear login nonce and throttle state.
260286
delete_user_meta( $user->ID, Two_Factor_Core::USER_META_NONCE_KEY );
261-
delete_user_meta( $user->ID, Two_Factor_Core::USER_PASSWORD_WAS_RESET_KEY );
262287
Two_Factor_Core::clear_login_rate_limit( $user );
263288

289+
// Preserve USER_PASSWORD_WAS_RESET_KEY: it records that the password was
290+
// reset after a compromise and drives the login notice explaining why the
291+
// old password stopped working. That is independent of 2FA configuration
292+
// and a rescued user should still see it after a reset.
293+
264294
// Clear provider-specific secrets for a clean baseline.
265295
if ( class_exists( 'Two_Factor_Totp' ) ) {
266296
Two_Factor_Totp::get_instance()->delete_user_totp_key( $user->ID );
@@ -286,6 +316,10 @@ private function disable_all_providers( $user, $assoc_args ) {
286316
);
287317
}
288318

319+
// The 2FA configuration changed, so drop every existing session and force
320+
// re-authentication, matching the profile-page behaviour.
321+
WP_Session_Tokens::get_instance( $user->ID )->destroy_all();
322+
289323
WP_CLI::success(
290324
sprintf(
291325
/* translators: 1: comma-separated provider names, 2: user login */
@@ -389,7 +423,7 @@ public function enable( $args, $assoc_args ) {
389423
WP_CLI::error(
390424
sprintf(
391425
/* translators: %s: provider class name */
392-
__( 'Provider %s requires a pre-shared secret and cannot be enabled from the CLI. Set it up via the user profile page or the totp subcommand (Phase 3).', 'two-factor' ),
426+
__( 'Provider %s requires a pre-shared secret and cannot be enabled from the CLI. Set it up via the user profile page.', 'two-factor' ),
393427
$provider
394428
)
395429
);
@@ -402,7 +436,11 @@ public function enable( $args, $assoc_args ) {
402436
);
403437
}
404438

439+
$providers_before = Two_Factor_Core::get_enabled_providers_for_user( $user );
440+
405441
if ( Two_Factor_Core::enable_provider_for_user( $user->ID, $provider ) ) {
442+
$this->destroy_sessions_if_providers_changed( $user, $providers_before );
443+
406444
WP_CLI::success(
407445
sprintf(
408446
/* translators: 1: provider class name, 2: user login */
@@ -493,6 +531,21 @@ public function backup_codes( $args, $assoc_args ) {
493531
)
494532
);
495533

534+
// Enable the provider so the codes are actually usable at login, mirroring
535+
// rest_generate_codes(). Without this the codes are stored but the provider
536+
// is never offered, so the user is rejected at the 2FA step.
537+
$providers_before = Two_Factor_Core::get_enabled_providers_for_user( $user );
538+
if ( ! Two_Factor_Core::enable_provider_for_user( $user->ID, 'Two_Factor_Backup_Codes' ) ) {
539+
WP_CLI::error(
540+
sprintf(
541+
/* translators: %s: user login */
542+
__( 'Backup codes were generated but the provider could not be enabled for user %s.', 'two-factor' ),
543+
$user->user_login
544+
)
545+
);
546+
}
547+
$this->destroy_sessions_if_providers_changed( $user, $providers_before );
548+
496549
WP_CLI::log(
497550
sprintf(
498551
/* translators: 1: number of codes, 2: user login */

‎TESTS.md‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ npm run composer -- test -- --group email
3535
npm run composer -- test -- --group backup-codes
3636
npm run composer -- test -- --group providers
3737
npm run composer -- test -- --group core
38+
npm run composer -- test -- --group cli
3839

3940
# Run a single file
4041
npm run composer -- test -- tests/providers/class-two-factor-totp.php
@@ -56,7 +57,7 @@ The largest test file. Covers the full authentication lifecycle managed by `Two_
5657
- Provider registration and retrieval (`get_providers`, `get_enabled_providers_for_user`, `get_available_providers_for_user`, `get_primary_provider_for_user`)
5758
- Login interception (`filter_authenticate`, `show_two_factor_login`, `process_provider`)
5859
- Login nonce creation, verification, and deletion
59-
- Rate limiting (`get_user_time_delay`, `is_user_rate_limited`)
60+
- Rate limiting (`get_user_time_delay`, `is_user_rate_limited`, `clear_login_rate_limit`)
6061
- Session management: two-factor factored vs. non-factored sessions, session destruction on 2FA enable/disable, revalidation
6162
- Password reset flow (compromise detection, email notifications, reset notices)
6263
- REST API permission callbacks (`rest_api_can_edit_user_and_update_two_factor_options`)
@@ -154,7 +155,25 @@ Tests `Two_Factor_Dummy_Secure` (a fixture that always _fails_ authentication, u
154155
- `validate_authentication` always returns false
155156
- `two_factor_provider_classname` filter
156157

158+
### WP-CLI Commands — `tests/cli/class-two-factor-cli-command.php`
159+
160+
**Class:** `Tests_Two_Factor_CLI_Command` · **Group:** `cli`
161+
Tests the `Two_Factor_CLI_Command` WP-CLI command class. The WP-CLI runtime is
162+
not loaded during PHPUnit, so the suite loads lightweight test doubles for
163+
`WP_CLI`, `WP_CLI_Command`, and the `WP_CLI\Utils` helpers (see Test Helpers)
164+
that capture output for assertions and throw on `error()`/`confirm()`:
165+
166+
- User resolution by ID, login, and email; "user not found" errors
167+
- `status` — output for users with and without 2FA, backup-code count, `--format` passthrough
168+
- `list-providers` — registered providers listed, `--format` passthrough
169+
- `enable` — enabling secret-free providers; session destruction on change; refusing TOTP (no stale "Phase 3" pointer) and backup codes; unknown provider and missing-argument errors
170+
- `disable` (single provider) — removal leaves others intact, session destruction on change, idempotent no-op, confirmation required without `--yes`
171+
- `disable` (all) — full reset clears providers/throttle/nonce state and destroys sessions, preserves the compromised-password-reset flag, idempotent no-op, stale-meta cleanup guarding the fail-closed email fallback, confirmation required without `--yes`
172+
- `backup-codes generate` — default and `--count` code counts, regeneration replaces the set, enables the provider so codes are usable at login, session destruction when first enabled, unknown-action and missing-argument errors
173+
- `unlock` — clears the login throttle for a rate-limited user; no-op message otherwise
174+
157175
## Test Helpers
158176

159177
- **`tests/bootstrap.php`** — Locates the WordPress test library (via `WP_TESTS_DIR` env var, relative path, or `/tmp/wordpress-tests-lib`), loads the plugin via `muplugins_loaded`, then boots the WP test environment.
160178
- **`tests/class-two-factor-dummy-secure.php`** — Defines `Two_Factor_Dummy_Secure`, a test-only provider class that spoofs the key of `Two_Factor_Dummy` but always fails `validate_authentication`. Used by `Tests_Two_Factor_Dummy_Secure` and some core tests.
179+
- **`tests/cli/`** — WP-CLI test doubles loaded by `Tests_Two_Factor_CLI_Command`: `class-wp-cli-command.php` (empty base-class stub), `class-wp-cli.php` (captures output into `WP_CLI::$logger`, throws on `error()`/`confirm()`), `class-wp-cli-mock-exit-exception.php` (stands in for a process exit), and `wp-cli-utils.php` (`WP_CLI\Utils\get_flag_value()` and `format_items()`).

‎tests/class-two-factor-core.php‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -748,6 +748,30 @@ public function test_is_user_rate_limited() {
748748
$this->assertFalse( Two_Factor_Core::is_user_rate_limited( $user ) );
749749
}
750750

751+
/**
752+
* Test that clearing the login rate limit removes the throttle state.
753+
*
754+
* @covers Two_Factor_Core::clear_login_rate_limit
755+
*/
756+
public function test_clear_login_rate_limit() {
757+
$user = $this->get_dummy_user();
758+
759+
// Put the user into a rate-limited state.
760+
update_user_meta( $user->ID, Two_Factor_Core::USER_FAILED_LOGIN_ATTEMPTS_KEY, 5 );
761+
update_user_meta( $user->ID, Two_Factor_Core::USER_RATE_LIMIT_KEY, time() );
762+
$this->assertTrue( Two_Factor_Core::is_user_rate_limited( $user ) );
763+
764+
Two_Factor_Core::clear_login_rate_limit( $user );
765+
766+
$this->assertFalse( Two_Factor_Core::is_user_rate_limited( $user ) );
767+
$this->assertEmpty( get_user_meta( $user->ID, Two_Factor_Core::USER_RATE_LIMIT_KEY, true ) );
768+
$this->assertEmpty( get_user_meta( $user->ID, Two_Factor_Core::USER_FAILED_LOGIN_ATTEMPTS_KEY, true ) );
769+
770+
// Clearing an already-clean user is a harmless no-op.
771+
Two_Factor_Core::clear_login_rate_limit( $user );
772+
$this->assertFalse( Two_Factor_Core::is_user_rate_limited( $user ) );
773+
}
774+
751775
/**
752776
* Test that the "invalid login attempts have occurred" login notice works as expected.
753777
*

0 commit comments

Comments
 (0)