diff --git a/CLI/class-two-factor-cli-command.php b/CLI/class-two-factor-cli-command.php index c81bf80b..0823fbca 100644 --- a/CLI/class-two-factor-cli-command.php +++ b/CLI/class-two-factor-cli-command.php @@ -216,6 +216,20 @@ public function disable( $args, $assoc_args ) { private function disable_single_provider( $user, $provider, $assoc_args ) { $enabled = Two_Factor_Core::get_enabled_providers_for_user( $user ); + // Reject unknown provider keys up front. Without this a typo or the wrong + // casing (Two_Factor_TOTP) would fall through to the "not enabled" branch + // below and exit zero, while the real provider stayed enabled. + if ( ! array_key_exists( $provider, Two_Factor_Core::get_supported_providers_for_user( $user ) ) ) { + WP_CLI::error( + sprintf( + /* translators: 1: provider class name, 2: user login */ + __( 'Unknown provider "%1$s". Run "wp two-factor list-providers" to see the registered providers, or "wp two-factor disable %2$s" to clear all two-factor state.', 'two-factor' ), + $provider, + $user->user_login + ) + ); + } + if ( ! in_array( $provider, $enabled, true ) ) { WP_CLI::success( sprintf( @@ -268,6 +282,65 @@ private function disable_single_provider( $user, $provider, $assoc_args ) { } } + /** + * User meta keys holding two-factor state that a full reset clears. + * + * Two_Factor_Core::USER_PASSWORD_WAS_RESET_KEY is deliberately absent: it + * records a password compromise rather than 2FA configuration and is + * preserved across a reset. + * + * @since 0.17.0 + * + * @return string[] Meta key names. + */ + private function two_factor_state_meta_keys() { + $keys = array( + Two_Factor_Core::ENABLED_PROVIDERS_USER_META_KEY, + Two_Factor_Core::PROVIDER_USER_META_KEY, + Two_Factor_Core::USER_META_NONCE_KEY, + Two_Factor_Core::USER_RATE_LIMIT_KEY, + Two_Factor_Core::USER_FAILED_LOGIN_ATTEMPTS_KEY, + ); + + if ( class_exists( 'Two_Factor_Totp' ) ) { + $keys[] = Two_Factor_Totp::SECRET_META_KEY; + $keys[] = Two_Factor_Totp::LAST_SUCCESSFUL_LOGIN_META_KEY; + } + + if ( class_exists( 'Two_Factor_Backup_Codes' ) ) { + $keys[] = Two_Factor_Backup_Codes::BACKUP_CODES_META_KEY; + } + + if ( class_exists( 'Two_Factor_Email' ) ) { + $keys[] = Two_Factor_Email::TOKEN_META_KEY; + $keys[] = Two_Factor_Email::TOKEN_META_KEY_TIMESTAMP; + } + + return $keys; + } + + /** + * Whether a user carries any two-factor state a full reset would clear. + * + * Active sessions are intentionally not treated as two-factor state, so a + * reset against an account that never used 2FA stays a true no-op instead of + * logging the user out. + * + * @since 0.17.0 + * + * @param WP_User $user Target user. + * @return bool True when there is something to clean up. + */ + private function has_residual_two_factor_state( $user ) { + foreach ( $this->two_factor_state_meta_keys() as $meta_key ) { + if ( ! empty( get_user_meta( $user->ID, $meta_key, true ) ) ) { + return true; + } + } + + return false; + } + /** * Disable all 2FA providers and clean up all residual state for a user. * @@ -278,9 +351,12 @@ private function disable_single_provider( $user, $provider, $assoc_args ) { */ private function disable_all_providers( $user, $assoc_args ) { $enabled = Two_Factor_Core::get_enabled_providers_for_user( $user ); - $raw = get_user_meta( $user->ID, Two_Factor_Core::ENABLED_PROVIDERS_USER_META_KEY, true ); - if ( empty( $enabled ) && empty( $raw ) ) { + // Gate on all residual state, not just the enabled-providers list. An + // account can carry a TOTP secret, recovery-code hashes, a login nonce or + // an active throttle while that list is empty; returning early here would + // report "already disabled" and leave every one of them in place. + if ( ! $this->has_residual_two_factor_state( $user ) ) { WP_CLI::success( sprintf( /* translators: %s: user login */ @@ -564,6 +640,19 @@ public function backup_codes( $args, $assoc_args ) { WP_CLI::error( __( 'The Two_Factor_Backup_Codes provider is not available.', 'two-factor' ) ); } + // Confirm the provider can actually be enabled before writing anything. + // generate_codes() replaces the stored hashes, so failing afterwards would + // destroy the old codes and leave unusable new ones the operator never saw. + if ( ! array_key_exists( 'Two_Factor_Backup_Codes', Two_Factor_Core::get_supported_providers_for_user( $user ) ) ) { + WP_CLI::error( + sprintf( + /* translators: %s: user login */ + __( 'The Two_Factor_Backup_Codes provider is not registered for user %s, so generated codes could not be used. No codes were changed.', 'two-factor' ), + $user->user_login + ) + ); + } + $raw_count = WP_CLI\Utils\get_flag_value( $assoc_args, 'count', (string) Two_Factor_Backup_Codes::NUMBER_OF_CODES ); if ( ! is_scalar( $raw_count ) ) { WP_CLI::error( @@ -627,10 +716,15 @@ public function backup_codes( $args, $assoc_args ) { // is never offered, so the user is rejected at the 2FA step. $providers_before = Two_Factor_Core::get_enabled_providers_for_user( $user ); if ( ! Two_Factor_Core::enable_provider_for_user( $user->ID, 'Two_Factor_Backup_Codes' ) ) { + // Unreachable via the pre-flight check above, but if enablement still + // fails the freshly written hashes are unusable — drop them rather than + // leaving unusable recovery material behind. + delete_user_meta( $user->ID, Two_Factor_Backup_Codes::BACKUP_CODES_META_KEY ); + WP_CLI::error( sprintf( /* translators: %s: user login */ - __( 'Backup codes were generated but the provider could not be enabled for user %s.', 'two-factor' ), + __( 'Backup codes could not be enabled for user %s. No codes were stored.', 'two-factor' ), $user->user_login ) ); diff --git a/TESTS.md b/TESTS.md index a7ec1b15..f5031513 100644 --- a/TESTS.md +++ b/TESTS.md @@ -167,9 +167,9 @@ that capture output for assertions and throw on `error()`/`confirm()`: - `status` — output for users with and without 2FA, backup-code count, `--format` passthrough - `list-providers` — registered providers listed, `--format` passthrough - `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 -- `disable` (single provider) — removal leaves others intact, session destruction on change, idempotent no-op, confirmation required without `--yes` -- `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` -- `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 +- `disable` (single provider) — removal leaves others intact (TOTP also drops its stored secret), session destruction on change, idempotent no-op, unregistered/mis-cased provider keys fail instead of reporting a false success, confirmation required without `--yes` +- `disable` (all) — full reset clears providers/throttle/nonce state and destroys sessions, also cleans residual secrets and lockout state when the enabled-providers list is already empty, preserves the compromised-password-reset flag, idempotent no-op, stale-meta cleanup guarding the fail-closed email fallback, confirmation required without `--yes` +- `backup-codes generate` — default and `--count` code counts, regeneration replaces the set, enables the provider so codes are usable at login, stores no hashes when the provider cannot be enabled, session destruction when first enabled, unknown-action and missing-argument errors - `unlock` — clears the login throttle for a rate-limited user; no-op message otherwise ## Test Helpers diff --git a/phpcs.xml.dist b/phpcs.xml.dist index 5d16e868..758510ac 100644 --- a/phpcs.xml.dist +++ b/phpcs.xml.dist @@ -40,6 +40,11 @@ tests/providers/*.php + + + CLI/*.php + + */wordpress/* */dist/* */includes/* diff --git a/tests/cli/class-two-factor-cli-command.php b/tests/cli/class-two-factor-cli-command.php index bf7806ee..d632cb3a 100644 --- a/tests/cli/class-two-factor-cli-command.php +++ b/tests/cli/class-two-factor-cli-command.php @@ -433,7 +433,8 @@ public function test_disable_single_totp_clears_stored_secret() { $this->enable_provider( 'Two_Factor_Totp' ); $totp = Two_Factor_Totp::get_instance(); - $this->assertTrue( $totp->set_user_totp_key( $this->user->ID, Two_Factor_Totp::generate_key() ) ); + // update_user_meta() returns the new meta ID (int) on insert, true on update. + $this->assertNotFalse( $totp->set_user_totp_key( $this->user->ID, Two_Factor_Totp::generate_key() ) ); $this->assertNotSame( '', $totp->get_user_totp_key( $this->user->ID ) ); $this->command->disable( array( 'cli_test_user', 'Two_Factor_Totp' ), array( 'yes' => true ) ); @@ -467,6 +468,31 @@ public function test_disable_single_provider_not_enabled() { $this->assertStringContainsString( 'no changes made', $this->last_message( 'success' ) ); } + /** + * Disabling an unregistered provider key fails loudly instead of reporting success. + * + * A mis-typed or mis-cased class name must not exit zero with "no changes + * made" while the real provider quietly stays enabled with its secret. + * + * @covers Two_Factor_CLI_Command::disable + */ + public function test_disable_single_provider_unknown_key_errors() { + $this->enable_provider( 'Two_Factor_Totp' ); + + $message = $this->assert_command_aborts( + function () { + $this->command->disable( array( 'cli_test_user', 'Two_Factor_TOTP' ), array( 'yes' => true ) ); + } + ); + + $this->assertStringContainsString( 'Two_Factor_TOTP', $message ); + $this->assertContains( + 'Two_Factor_Totp', + Two_Factor_Core::get_enabled_providers_for_user( $this->user ), + 'The correctly-cased provider must stay enabled after a failed disable.' + ); + } + /** * Disabling a single provider requires confirmation without --yes. * @@ -555,6 +581,40 @@ public function test_disable_all_providers_when_already_disabled() { $this->assertStringContainsString( 'already disabled', $this->last_message( 'success' ) ); } + /** + * A full reset clears residual state even when no providers are enabled. + * + * An account can still carry a TOTP secret, recovery-code hashes, a login + * nonce and an active throttle while the enabled-providers list is empty. + * Reporting "already disabled" and returning would leave all of it behind. + * + * @covers Two_Factor_CLI_Command::disable + */ + public function test_disable_all_providers_clears_residual_state_without_enabled_providers() { + $totp = Two_Factor_Totp::get_instance(); + $totp->set_user_totp_key( $this->user->ID, Two_Factor_Totp::generate_key() ); + Two_Factor_Backup_Codes::get_instance()->generate_codes( $this->user, array( 'method' => 'replace' ) ); + update_user_meta( $this->user->ID, Two_Factor_Core::USER_META_NONCE_KEY, array( 'key' => 'nonce-value' ) ); + update_user_meta( $this->user->ID, Two_Factor_Core::USER_RATE_LIMIT_KEY, time() ); + update_user_meta( $this->user->ID, Two_Factor_Core::USER_FAILED_LOGIN_ATTEMPTS_KEY, 7 ); + $this->create_user_session(); + + $this->assertEmpty( + Two_Factor_Core::get_enabled_providers_for_user( $this->user ), + 'This scenario requires an empty enabled-providers list.' + ); + + $this->command->disable( array( 'cli_test_user' ), array( 'yes' => true ) ); + + $this->assertSame( '', $totp->get_user_totp_key( $this->user->ID ) ); + $this->assertSame( 0, Two_Factor_Backup_Codes::codes_remaining_for_user( $this->user ) ); + $this->assertEmpty( get_user_meta( $this->user->ID, Two_Factor_Core::USER_META_NONCE_KEY, true ) ); + $this->assertEmpty( get_user_meta( $this->user->ID, Two_Factor_Core::USER_RATE_LIMIT_KEY, true ) ); + $this->assertEmpty( get_user_meta( $this->user->ID, Two_Factor_Core::USER_FAILED_LOGIN_ATTEMPTS_KEY, true ) ); + $this->assertSame( 0, $this->count_user_sessions() ); + $this->assertStringContainsString( 'All 2FA disabled', $this->last_message( 'success' ) ); + } + /** * A full disable clears stale meta even when the provider class no longer exists. * @@ -777,7 +837,42 @@ function () { } /** - * An unknown backup-codes action aborts with an error. + * No backup-code hashes are stored when the provider cannot be enabled. + * + * Generating first and enabling second leaves unusable hashes behind when + * enablement fails, with no plaintext codes shown to the operator. + * + * @covers Two_Factor_CLI_Command::backup_codes + */ + public function test_backup_codes_generate_does_not_store_hashes_when_provider_unavailable() { + $deregister = function ( $providers ) { + unset( $providers['Two_Factor_Backup_Codes'] ); + + return $providers; + }; + + add_filter( 'two_factor_providers', $deregister ); + + try { + $this->assert_command_aborts( + function () { + $this->command->backup_codes( array( 'generate', 'cli_test_user' ), array() ); + } + ); + } finally { + remove_filter( 'two_factor_providers', $deregister ); + } + + $this->assertSame( + 0, + Two_Factor_Backup_Codes::codes_remaining_for_user( $this->user ), + 'Unusable hashes must not be left behind when the provider cannot be enabled.' + ); + $this->assertEmpty( Two_Factor_Core::get_enabled_providers_for_user( $this->user ) ); + } + + /** + * An unknown backup-codes action is rejected. * * @covers Two_Factor_CLI_Command::backup_codes */