From dd72311e66d60530062b64fcb8862b582e5eea2a Mon Sep 17 00:00:00 2001 From: Andrea Grassi Date: Wed, 5 Nov 2025 18:28:19 +0100 Subject: [PATCH 1/8] exclude node modules from phpstan --- phpstan.neon | 2 ++ 1 file changed, 2 insertions(+) diff --git a/phpstan.neon b/phpstan.neon index f0401368..c9f965d2 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -10,6 +10,8 @@ parameters: - email - class-loader.php - vip-security-boost.php + excludePaths: + - %rootDir%/../../../tests/e2e/node_modules scanDirectories: - mu-plugins scanFiles: From 5b1108ca6a743f1091d4e5bcca8a128d0fcc0ef2 Mon Sep 17 00:00:00 2001 From: Andrea Grassi Date: Wed, 5 Nov 2025 18:29:25 +0100 Subject: [PATCH 2/8] Improve 2FA count performance. --- .../class-forced-mfa-users.php | 51 ++++-- utils/class-users-query-utils.php | 154 +++++++++++------- 2 files changed, 131 insertions(+), 74 deletions(-) diff --git a/modules/forced-mfa-users/class-forced-mfa-users.php b/modules/forced-mfa-users/class-forced-mfa-users.php index 9f2dc596..a6f1a8d1 100644 --- a/modules/forced-mfa-users/class-forced-mfa-users.php +++ b/modules/forced-mfa-users/class-forced-mfa-users.php @@ -246,7 +246,6 @@ private static function get_mfa_disabled_count( $blog_id = null ) { 'fields' => 'ID', // phpcs:ignore WordPressVIPMinimum.Performance.WPQueryParams.PostNotIn_exclude -- Excluding a potentially small, known set of users (skipped + ID 1) 'exclude' => $skipped_user_ids, - 'number' => -1, // Get all relevant users ]; // Use native capability filtering if capabilities are configured @@ -267,20 +266,14 @@ private static function get_mfa_disabled_count( $blog_id = null ) { $args['role__in'] = $roles; } - // Use our utility method that properly handles network-wide capability filtering - $user_ids = Users_Query_Utils::query_users_with_capability_filtering( + $args['meta_query'] = self::get_users_without_two_factor_meta_query(); + + $mfa_disabled_count = Users_Query_Utils::query_users_with_capability_filtering( $args, $blog_id, - false // return user IDs, not count + true ); - $mfa_disabled_count = 0; - foreach ( $user_ids as $user_id ) { - if ( ! \Two_Factor_Core::is_user_using_two_factor( $user_id ) ) { - ++$mfa_disabled_count; - } - } - // Cache the result // phpcs:ignore WordPressVIPMinimum.Performance.LowExpiryCacheTime.CacheTimeUndetermined wp_cache_set( $cache_key, $mfa_disabled_count, self::MFA_COUNT_CACHE_GROUP, self::MFA_COUNT_CACHE_TTL ); @@ -288,6 +281,42 @@ private static function get_mfa_disabled_count( $blog_id = null ) { return $mfa_disabled_count; } + /** + * Build a meta query that matches users without an enabled Two Factor provider. + * + * Users are considered 2FA-disabled when `_two_factor_enabled_providers` is missing, + * empty, or serialized as an empty array (`a:0:{}`). + * + * This might not return the exact results, since the original Two Factor plugin also applies a filter, but it's still a good indication, which is what we care about. + * + * @return array + */ + private static function get_users_without_two_factor_meta_query() { + $meta_key = \Two_Factor_Core::ENABLED_PROVIDERS_USER_META_KEY; + + $empty_values = apply_filters( + 'vip_security_forced_mfa_empty_enabled_providers_values', + [ '', 'a:0:{}', 'N;', 'b:0;' ] + ); + + $empty_values = array_unique( array_map( 'strval', (array) $empty_values ) ); + + $meta_query = [ + 'relation' => 'OR', + [ + 'key' => $meta_key, + 'compare' => 'NOT EXISTS', + ], + [ + 'key' => $meta_key, + 'value' => $empty_values, + 'compare' => 'IN', + ], + ]; + + return $meta_query; + } + /** * Clear the MFA disabled count cache. * Called when user MFA settings or roles change. diff --git a/utils/class-users-query-utils.php b/utils/class-users-query-utils.php index a3ec6f5a..c48b25f7 100644 --- a/utils/class-users-query-utils.php +++ b/utils/class-users-query-utils.php @@ -32,100 +32,128 @@ public static function fix_found_users_query( $sql, $query ) { } /** - * Perform a user query with proper capability/role filtering for network-wide queries. - * - * This method solves the WordPress core issue where WP_User_Query ignores - * capability__in and role__in parameters when blog_id=0 is used. - * - * Similar to fix_found_users_query, this leverages WP_User_Query to build - * the base SQL and then modifies the WHERE clause to add network-wide - * capability/role filtering. + * Prepare a WP_User_Query instance and capability clause for re-use in custom SQL builders. * - * @param array $query_args WP_User_Query arguments. Should include capability__in or role__in. - * @param int $blog_id Blog ID. Use 0 for network-wide queries. - * @param bool $count_only Whether to return only the count (true) or user IDs (false). - * @return int|array Returns count (int) if $count_only is true, otherwise array of user IDs. + * @param array $query_args WP_User_Query arguments. + * @param int|null $blog_id Blog ID. Use 0 for network-wide queries. + * @return array { + * @type \WP_User_Query $query Prepared WP_User_Query instance. + * @type string $capability_where Additional capability WHERE clause for network-wide queries. + * @type bool $is_network True when the query targets the entire network (blog_id = 0). + * } */ - public static function query_users_with_capability_filtering( $query_args, $blog_id = null, $count_only = true ) { - global $wpdb; + public static function get_prepared_user_query_data( $query_args, $blog_id = null ) { Role_Sanitizer::maybe_register_role_sanitizers(); try { - // Single blog query + $blog_id = null === $blog_id ? get_current_blog_id() : (int) $blog_id; + if ( ! is_multisite() || 0 !== $blog_id ) { $query_args['blog_id'] = $blog_id; $query_args['fields'] = 'ID'; - if ( $count_only ) { - $query_args['count_total'] = true; - $query_args['number'] = 1; // We only need the count - } - $user_query = new \WP_User_Query(); $user_query->prepare_query( $query_args ); - if ( $count_only ) { - // phpcs:ignore WordPressVIPMinimum.Variables.RestrictedVariables.user_meta__wpdb__users - $user_query->query_fields = 'COUNT(DISTINCT ' . $wpdb->users . '.ID)'; - } - - $request = - "SELECT {$user_query->query_fields} - {$user_query->query_from} - {$user_query->query_where} - {$user_query->query_orderby} - {$user_query->query_limit}"; - - if ( $count_only ) { - // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared,WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching - return (int) $wpdb->get_var( $request ); - } else { - // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared,WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching - return array_map( 'intval', $wpdb->get_col( $request ) ); - } + return [ + 'query' => $user_query, + 'capability_where' => '', + 'is_network' => false, + ]; } - // Network-wide query $capabilities = $query_args['capability__in'] ?? []; $roles = $query_args['role__in'] ?? []; - // Remove capability/role filters from query args and let WP_User_Query build the base query $base_query_args = $query_args; unset( $base_query_args['capability__in'], $base_query_args['role__in'] ); $base_query_args['fields'] = 'ID'; $base_query_args['blog_id'] = 0; - // Create WP_User_Query to get the base SQL clauses - $temp_query = new \WP_User_Query(); - $temp_query->prepare_query( $base_query_args ); + $user_query = new \WP_User_Query(); + $user_query->prepare_query( $base_query_args ); - // Build our network-wide capability filtering clause $capability_where = self::build_network_capability_where_clause( $capabilities, $roles ); - if ( empty( $capability_where ) ) { - // No capability/role filtering needed, use the temp query results - return $count_only ? $temp_query->get_total() : array_map( 'intval', $temp_query->get_results() ); + return [ + 'query' => $user_query, + 'capability_where' => $capability_where, + 'is_network' => true, + ]; + } finally { + Role_Sanitizer::unregister_role_sanitizers(); + } + } + + /** + * Perform a user query with proper capability/role filtering for network-wide queries. + * + * This method solves the WordPress core issue where WP_User_Query ignores + * capability__in and role__in parameters when blog_id=0 is used. + * + * Similar to fix_found_users_query, this leverages WP_User_Query to build + * the base SQL and then modifies the WHERE clause to add network-wide + * capability/role filtering. + * + * @param array $query_args WP_User_Query arguments. Should include capability__in or role__in. + * @param int $blog_id Blog ID. Use 0 for network-wide queries. + * @param bool $count_only Whether to return only the count (true) or user IDs (false). + * @return int|array Returns count (int) if $count_only is true, otherwise array of user IDs. + */ + public static function query_users_with_capability_filtering( $query_args, $blog_id = null, $count_only = true ) { + global $wpdb; + + if ( $count_only ) { + $query_args['count_total'] = true; + } + + $prepared = self::get_prepared_user_query_data( $query_args, $blog_id ); + $user_query = $prepared['query']; + $is_network = $prepared['is_network']; + $capability_where = $prepared['capability_where']; + + if ( ! $is_network ) { + if ( $count_only ) { + // phpcs:ignore WordPressVIPMinimum.Variables.RestrictedVariables.user_meta__wpdb__users + $user_query->query_fields = 'COUNT(DISTINCT ' . $wpdb->users . '.ID)'; } - // Build the final query using WP_User_Query's SQL with our additional WHERE clause + $request = + "SELECT {$user_query->query_fields} + {$user_query->query_from} + {$user_query->query_where} + {$user_query->query_orderby} + {$user_query->query_limit}"; + if ( $count_only ) { - // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared -- SQL is built by WP_User_Query and internal methods - // phpcs:ignore WordPressVIPMinimum.Variables.RestrictedVariables.user_meta__wpdb__users -- Required for network-wide user capability filtering - $sql = "SELECT COUNT(DISTINCT {$wpdb->users}.ID) {$temp_query->query_from} {$temp_query->query_where} AND ({$capability_where})"; // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared,WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching - $result = $wpdb->get_var( $sql ); - return (int) $result; - } else { - // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared -- SQL is built by WP_User_Query and internal methods - // phpcs:ignore WordPressVIPMinimum.Variables.RestrictedVariables.user_meta__wpdb__users -- Required for network-wide user capability filtering - $sql = "SELECT DISTINCT {$wpdb->users}.ID {$temp_query->query_from} {$temp_query->query_where} AND ({$capability_where})"; - // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared,WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching - $results = $wpdb->get_col( $sql ); - return array_map( 'intval', $results ); + return (int) $wpdb->get_var( $request ); } - } finally { - Role_Sanitizer::unregister_role_sanitizers(); + + // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared,WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching + return array_map( 'intval', $wpdb->get_col( $request ) ); + } + + if ( empty( $capability_where ) ) { + return $count_only ? $user_query->get_total() : array_map( 'intval', $user_query->get_results() ); } + + // Build the final query using WP_User_Query's SQL with our additional WHERE clause + if ( $count_only ) { + // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared -- SQL is built by WP_User_Query and internal methods + // phpcs:ignore WordPressVIPMinimum.Variables.RestrictedVariables.user_meta__wpdb__users -- Required for network-wide user capability filtering + $sql = "SELECT COUNT(DISTINCT {$wpdb->users}.ID) {$user_query->query_from} {$user_query->query_where} AND ({$capability_where})"; + // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared,WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching + $result = $wpdb->get_var( $sql ); + return (int) $result; + } + + // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared -- SQL is built by WP_User_Query and internal methods + // phpcs:ignore WordPressVIPMinimum.Variables.RestrictedVariables.user_meta__wpdb__users -- Required for network-wide user capability filtering + $sql = "SELECT DISTINCT {$wpdb->users}.ID {$user_query->query_from} {$user_query->query_where} AND ({$capability_where})"; + // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared,WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching + $results = $wpdb->get_col( $sql ); + return array_map( 'intval', $results ); } /** From 864e553efdf47145f4e40b879256ab72a04cc5ee Mon Sep 17 00:00:00 2001 From: Andrea Grassi Date: Wed, 5 Nov 2025 18:36:39 +0100 Subject: [PATCH 3/8] Revert "exclude node modules from phpstan" This reverts commit dd72311e66d60530062b64fcb8862b582e5eea2a. --- phpstan.neon | 2 -- 1 file changed, 2 deletions(-) diff --git a/phpstan.neon b/phpstan.neon index c9f965d2..f0401368 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -10,8 +10,6 @@ parameters: - email - class-loader.php - vip-security-boost.php - excludePaths: - - %rootDir%/../../../tests/e2e/node_modules scanDirectories: - mu-plugins scanFiles: From 778c21bc47798084715714fe5378acaf17ab026c Mon Sep 17 00:00:00 2001 From: Andrea Grassi Date: Thu, 6 Nov 2025 18:22:48 +0100 Subject: [PATCH 4/8] Add SQL-based counting for inactive users with elevated permissions and enhance tests --- .../inactive-users/class-inactive-users.php | 173 +++++++++++++++--- tests/phpunit/test-inactive-users.php | 72 +++++++- 2 files changed, 215 insertions(+), 30 deletions(-) diff --git a/modules/inactive-users/class-inactive-users.php b/modules/inactive-users/class-inactive-users.php index 7aa4ee32..4db21dbd 100644 --- a/modules/inactive-users/class-inactive-users.php +++ b/modules/inactive-users/class-inactive-users.php @@ -318,46 +318,161 @@ public static function last_seen_order_by_query_args( $vars ) { return $vars; } + /** + * Count inactive users with elevated permissions. + * + * This method uses pure SQL to count users who: + * 1. Have elevated roles or capabilities (configured via settings) + * 2. Are active (not spam, deleted, or have user_status != 0) + * 3. Registered before the inactivity threshold + * 4. Either have not been seen since the threshold, or never had a last_seen recorded + * + * The query structure leverages WordPress's WP_User_Query to generate the capability/role + * filtering SQL, then uses that as a subquery to filter users in the main count query. + * + * Results are cached with a 5-minute TTL to improve performance. + * + * Example SQL (single site with administrator role): + * + * SELECT COUNT(DISTINCT u.ID) + * FROM wp_users u + * LEFT JOIN wp_usermeta m_last_seen + * ON u.ID = m_last_seen.user_id + * AND m_last_seen.meta_key = 'wpvip_last_seen' + * WHERE u.user_status=0 AND u.spam=0 and u.deleted=0 + * AND u.user_registered < '2024-08-06 12:00:00' + * AND ( + * (m_last_seen.meta_value IS NOT NULL AND CAST(m_last_seen.meta_value AS UNSIGNED) < 1722945600) + * OR m_last_seen.meta_value IS NULL + * ) + * AND u.ID IN ( + * SELECT DISTINCT wp_users.ID + * FROM wp_users + * INNER JOIN wp_usermeta ON wp_users.ID = wp_usermeta.user_id + * WHERE 1=1 + * AND (wp_usermeta.meta_key = 'wp_capabilities' + * AND wp_usermeta.meta_value LIKE '%\"administrator\"%') + * ) + * + * Example SQL (network-wide with administrator role): + * + * SELECT COUNT(DISTINCT u.ID) + * FROM wp_users u + * LEFT JOIN wp_usermeta m_last_seen + * ON u.ID = m_last_seen.user_id + * AND m_last_seen.meta_key = 'wpvip_last_seen' + * WHERE u.user_status=0 AND u.spam=0 and u.deleted=0 + * AND u.user_registered < '2024-08-06 12:00:00' + * AND ( + * (m_last_seen.meta_value IS NOT NULL AND CAST(m_last_seen.meta_value AS UNSIGNED) < 1722945600) + * OR m_last_seen.meta_value IS NULL + * ) + * AND u.ID IN ( + * SELECT DISTINCT user_id + * FROM wp_usermeta + * WHERE meta_key LIKE 'wp%_capabilities' + * AND ( + * meta_value LIKE '%\"administrator\";b:1%' + * ) + * ) + * + * @param int|null $blog_id Optional. Blog ID to query. Use 0 for network-wide queries. Default is current blog. + * @return int Count of inactive users. + */ public static function get_inactive_users_count( $blog_id = null ) { + global $wpdb; + $blog_id = $blog_id ?? get_current_blog_id(); - // Use global cache for network-wide queries (blog_id = 0) - if ( 0 === $blog_id ) { - $cache_key = self::get_inactive_users_count_cache_key( $blog_id ); - $cached_count = wp_cache_get( $cache_key, self::LAST_SEEN_CACHE_GROUP ); - if ( false !== $cached_count ) { - return $cached_count; - } + $cache_key = self::get_inactive_users_count_cache_key( $blog_id ); + $cached_count = wp_cache_get( $cache_key, self::LAST_SEEN_CACHE_GROUP ); + + if ( false !== $cached_count ) { + return $cached_count; } - /** - * We're doing two separate queries to avoid the query getting too complex and slow since excluding the last_seen meta would add an extra join - * multiplying the complexity of the query beneath it - */ - // Use our utility method that properly handles network-wide capability filtering - $inactive_users_with_last_seen_count = \Automattic\VIP\Security\Utils\Users_Query_Utils::query_users_with_capability_filtering( - self::get_inactive_users_query_args( 'with_last_seen' ), - $blog_id, - true // count only - ); - $inactive_users_without_last_seen_count = 0; - if ( self::is_release_date_older_than_cutoff() ) { - $inactive_users_without_last_seen_count = \Automattic\VIP\Security\Utils\Users_Query_Utils::query_users_with_capability_filtering( - self::get_inactive_users_query_args( 'without_last_seen' ), - $blog_id, - true // count only - ); + $inact_ts = self::get_inactivity_timestamp(); + $release_ts = static::get_last_seen_release_date_timestamp(); + + // Build the inactivity date for user_registered comparison + $inactivity_date = gmdate( 'Y-m-d H:i:s', $inact_ts ); + + // Use WordPress's WP_User_Query to build the capability/role filtering SQL + $capability_query_args = [ + 'fields' => 'ID', + 'count_total' => false, // Prevent SQL_CALC_FOUND_ROWS which doesn't work in subqueries + ]; + + // Add capability or role filtering + if ( ! empty( self::$elevated_capabilities ) ) { + $capability_query_args['capability__in'] = self::$elevated_capabilities; + } else { + $capability_query_args['role__in'] = Capability_Utils::normalize_roles_input( self::$elevated_roles ); } - $count = $inactive_users_with_last_seen_count + $inactive_users_without_last_seen_count; + // Get the prepared query data using our utility method + $prepared = Users_Query_Utils::get_prepared_user_query_data( $capability_query_args, $blog_id ); + $user_query = $prepared['query']; + $is_network = $prepared['is_network']; + $capability_where = $prepared['capability_where']; - // Cache the result for global queries (blog_id = 0) - if ( 0 === $blog_id ) { - $cache_key = self::get_inactive_users_count_cache_key( $blog_id ); - wp_cache_set( $cache_key, $count, self::LAST_SEEN_CACHE_GROUP, self::INACTIVE_USERS_COUNT_CACHE_TTL ); // phpcs:ignore WordPressVIPMinimum.Performance.LowExpiryCacheTime.CacheTimeUndetermined + // Build the subquery for users with elevated permissions + // Remove SQL_CALC_FOUND_ROWS if present (not compatible with subqueries in MySQL 8+) + $query_fields = str_replace( 'SQL_CALC_FOUND_ROWS ', '', $user_query->query_fields ); + $users_with_caps_subquery = "SELECT DISTINCT {$query_fields} {$user_query->query_from} {$user_query->query_where}"; + + if ( $is_network && ! empty( $capability_where ) ) { + $users_with_caps_subquery .= " AND ({$capability_where})"; + } + + // Build the last_seen conditions + $last_seen_conditions = []; + + // Users with last_seen < inactivity threshold + $last_seen_conditions[] = $wpdb->prepare( '(m_last_seen.meta_value IS NOT NULL AND CAST(m_last_seen.meta_value AS UNSIGNED) < %d)', $inact_ts ); + + // Users without last_seen meta (only if release date is older than cutoff) + if ( $release_ts < $inact_ts ) { + $last_seen_conditions[] = 'm_last_seen.meta_value IS NULL'; } + $last_seen_where = '(' . implode( ' OR ', $last_seen_conditions ) . ')'; + $last_seen_meta_key = self::LAST_SEEN_META_KEY; + + // include only valid users. + $status_conditions = 'u.user_status = 0'; + if ( is_multisite() ) { + $status_conditions .= ' AND u.spam = 0 AND u.deleted = 0'; + } + + // Build the main SQL query + // phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared,WordPressVIPMinimum.Variables.RestrictedVariables.user_meta__wpdb__users + $sql = $wpdb->prepare( + "SELECT COUNT(DISTINCT u.ID) + FROM {$wpdb->users} u + LEFT JOIN {$wpdb->usermeta} m_last_seen + ON u.ID = m_last_seen.user_id + AND m_last_seen.meta_key = %s + WHERE {$status_conditions} + AND u.user_registered < %s + AND {$last_seen_where} + AND u.ID IN ({$users_with_caps_subquery})", + $last_seen_meta_key, + $inactivity_date + ); + // phpcs:enable WordPress.DB.PreparedSQL.InterpolatedNotPrepared,WordPressVIPMinimum.Variables.RestrictedVariables.user_meta__wpdb__users + + // Execute the query + // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared,WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching + $count = (int) $wpdb->get_var( $sql ); + + // Cache the result for global queries (blog_id = 0) + + $cache_key = self::get_inactive_users_count_cache_key( $blog_id ); + wp_cache_set( $cache_key, $count, self::LAST_SEEN_CACHE_GROUP, self::INACTIVE_USERS_COUNT_CACHE_TTL ); // phpcs:ignore WordPressVIPMinimum.Performance.LowExpiryCacheTime.CacheTimeUndetermined + + return $count; } diff --git a/tests/phpunit/test-inactive-users.php b/tests/phpunit/test-inactive-users.php index 7c17bce4..d800bc46 100644 --- a/tests/phpunit/test-inactive-users.php +++ b/tests/phpunit/test-inactive-users.php @@ -6,6 +6,8 @@ class InactiveUsersTest extends WP_UnitTestCase { private $user_id; private $elevated_roles = [ 'administrator' ]; private $considered_inactive_after_days = 90; + private $additional_user_ids = []; + private $original_release_timestamp; public function setUp(): void { parent::setUp(); @@ -27,17 +29,56 @@ public function setUp(): void { ], ]); } + $this->original_release_timestamp = get_option( Inactive_Users::LAST_SEEN_RELEASE_DATE_TIMESTAMP_OPTION_KEY, false ); // Let's assume the module has been activated today - add_option( Inactive_Users::LAST_SEEN_RELEASE_DATE_TIMESTAMP_OPTION_KEY, time() ); + update_option( Inactive_Users::LAST_SEEN_RELEASE_DATE_TIMESTAMP_OPTION_KEY, time() ); + update_user_meta( $this->user_id, Inactive_Users::LAST_SEEN_META_KEY, time() ); Inactive_Users::init(); } public function tearDown(): void { + foreach ( $this->additional_user_ids as $user_id ) { + wp_delete_user( $user_id ); + } + $this->additional_user_ids = []; + wp_delete_user( $this->user_id ); + + if ( false === $this->original_release_timestamp ) { + delete_option( Inactive_Users::LAST_SEEN_RELEASE_DATE_TIMESTAMP_OPTION_KEY ); + } else { + update_option( Inactive_Users::LAST_SEEN_RELEASE_DATE_TIMESTAMP_OPTION_KEY, $this->original_release_timestamp ); + } + parent::tearDown(); } + /** + * Test that get_inactive_users_count counts users with stale last_seen values. + */ + public function test_get_inactive_users_count_counts_users_with_stale_last_seen(): void { + $baseline_count = $this->get_fresh_inactive_users_count(); + + $user_id = $this->create_test_user(); + update_user_meta( $user_id, Inactive_Users::LAST_SEEN_META_KEY, strtotime( '-91 days' ) ); + + $this->assertSame( $baseline_count + 1, $this->get_fresh_inactive_users_count() ); + } + + /** + * Test that users without last_seen are counted once the release date predates the cutoff. + */ + public function test_get_inactive_users_count_counts_users_without_last_seen_after_release_cutoff(): void { + update_option( Inactive_Users::LAST_SEEN_RELEASE_DATE_TIMESTAMP_OPTION_KEY, strtotime( '-400 days' ) ); + $baseline_count = $this->get_fresh_inactive_users_count(); + + $user_id = $this->create_test_user(); + delete_user_meta( $user_id, Inactive_Users::LAST_SEEN_META_KEY ); + + $this->assertSame( $baseline_count + 1, $this->get_fresh_inactive_users_count() ); + } + /** * Test that add_last_seen_column_head adds a 'Last seen' column to the columns array */ @@ -1107,4 +1148,33 @@ public function test_application_password_authentication_inactive_user() { wp_delete_user( $user_id ); } + + /** + * Create a test user and ensure it is cleaned up after the test. + * + * @param array $overrides Optional. User factory overrides. + * @return int The created user ID. + */ + private function create_test_user( array $overrides = [] ): int { + $defaults = [ + 'role' => 'administrator', + 'user_registered' => gmdate( 'Y-m-d H:i:s', strtotime( '-120 days' ) ), + ]; + + $user_id = $this->factory->user->create( array_merge( $defaults, $overrides ) ); + $this->additional_user_ids[] = $user_id; + + return $user_id; + } + + /** + * Convenience wrapper to fetch a cache-bypassed inactive users count. + * + * @param int|null $blog_id Optional blog ID. + * @return int + */ + private function get_fresh_inactive_users_count( $blog_id = null ): int { + Inactive_Users::flush_cache(); + return Inactive_Users::get_inactive_users_count( $blog_id ); + } } From 9c672f0eeda66de727cc06a5c89cc2c3e3f8bed3 Mon Sep 17 00:00:00 2001 From: Andrea Grassi Date: Thu, 6 Nov 2025 18:25:54 +0100 Subject: [PATCH 5/8] fix: add PHPCS ignore comment for slow DB query in meta query for users without 2FA --- modules/forced-mfa-users/class-forced-mfa-users.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/forced-mfa-users/class-forced-mfa-users.php b/modules/forced-mfa-users/class-forced-mfa-users.php index a6f1a8d1..43d8db04 100644 --- a/modules/forced-mfa-users/class-forced-mfa-users.php +++ b/modules/forced-mfa-users/class-forced-mfa-users.php @@ -265,7 +265,7 @@ private static function get_mfa_disabled_count( $blog_id = null ) { } else { $args['role__in'] = $roles; } - + // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_query -- Required meta query to find users without 2FA enabled $args['meta_query'] = self::get_users_without_two_factor_meta_query(); $mfa_disabled_count = Users_Query_Utils::query_users_with_capability_filtering( From dbdc147e3a65272a81a162780fa109a49b0ea341 Mon Sep 17 00:00:00 2001 From: Andrea Grassi Date: Fri, 7 Nov 2025 11:05:34 +0100 Subject: [PATCH 6/8] reintroduce sds hook to sync data --- modules/inactive-users/class-inactive-users.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/inactive-users/class-inactive-users.php b/modules/inactive-users/class-inactive-users.php index 4db21dbd..4d91a89c 100644 --- a/modules/inactive-users/class-inactive-users.php +++ b/modules/inactive-users/class-inactive-users.php @@ -90,9 +90,8 @@ public static function init() { add_action( 'admin_init', [ __CLASS__, 'last_seen_unblock_action' ] ); } - // TODO - Fix performance issues with network-wide queries // Add SDS hook - // add_filter( 'vip_site_details_index_security_boost_data', [ __CLASS__, 'add_inactive_users_count_to_sds_payload' ] ); + add_filter( 'vip_site_details_index_security_boost_data', [ __CLASS__, 'add_inactive_users_count_to_sds_payload' ] ); } public static function maybe_fix_found_users_query() { From 7ec2a0ced70235496e523fedcd4180c3dfb0ae12 Mon Sep 17 00:00:00 2001 From: Andrea Grassi Date: Mon, 10 Nov 2025 14:10:23 +0100 Subject: [PATCH 7/8] Revert "Add SQL-based counting for inactive users with elevated permissions and enhance tests" This reverts commit 778c21bc47798084715714fe5378acaf17ab026c. --- .../inactive-users/class-inactive-users.php | 173 +++--------------- tests/phpunit/test-inactive-users.php | 72 +------- 2 files changed, 30 insertions(+), 215 deletions(-) diff --git a/modules/inactive-users/class-inactive-users.php b/modules/inactive-users/class-inactive-users.php index 4d91a89c..89823cf5 100644 --- a/modules/inactive-users/class-inactive-users.php +++ b/modules/inactive-users/class-inactive-users.php @@ -317,160 +317,45 @@ public static function last_seen_order_by_query_args( $vars ) { return $vars; } - /** - * Count inactive users with elevated permissions. - * - * This method uses pure SQL to count users who: - * 1. Have elevated roles or capabilities (configured via settings) - * 2. Are active (not spam, deleted, or have user_status != 0) - * 3. Registered before the inactivity threshold - * 4. Either have not been seen since the threshold, or never had a last_seen recorded - * - * The query structure leverages WordPress's WP_User_Query to generate the capability/role - * filtering SQL, then uses that as a subquery to filter users in the main count query. - * - * Results are cached with a 5-minute TTL to improve performance. - * - * Example SQL (single site with administrator role): - * - * SELECT COUNT(DISTINCT u.ID) - * FROM wp_users u - * LEFT JOIN wp_usermeta m_last_seen - * ON u.ID = m_last_seen.user_id - * AND m_last_seen.meta_key = 'wpvip_last_seen' - * WHERE u.user_status=0 AND u.spam=0 and u.deleted=0 - * AND u.user_registered < '2024-08-06 12:00:00' - * AND ( - * (m_last_seen.meta_value IS NOT NULL AND CAST(m_last_seen.meta_value AS UNSIGNED) < 1722945600) - * OR m_last_seen.meta_value IS NULL - * ) - * AND u.ID IN ( - * SELECT DISTINCT wp_users.ID - * FROM wp_users - * INNER JOIN wp_usermeta ON wp_users.ID = wp_usermeta.user_id - * WHERE 1=1 - * AND (wp_usermeta.meta_key = 'wp_capabilities' - * AND wp_usermeta.meta_value LIKE '%\"administrator\"%') - * ) - * - * Example SQL (network-wide with administrator role): - * - * SELECT COUNT(DISTINCT u.ID) - * FROM wp_users u - * LEFT JOIN wp_usermeta m_last_seen - * ON u.ID = m_last_seen.user_id - * AND m_last_seen.meta_key = 'wpvip_last_seen' - * WHERE u.user_status=0 AND u.spam=0 and u.deleted=0 - * AND u.user_registered < '2024-08-06 12:00:00' - * AND ( - * (m_last_seen.meta_value IS NOT NULL AND CAST(m_last_seen.meta_value AS UNSIGNED) < 1722945600) - * OR m_last_seen.meta_value IS NULL - * ) - * AND u.ID IN ( - * SELECT DISTINCT user_id - * FROM wp_usermeta - * WHERE meta_key LIKE 'wp%_capabilities' - * AND ( - * meta_value LIKE '%\"administrator\";b:1%' - * ) - * ) - * - * @param int|null $blog_id Optional. Blog ID to query. Use 0 for network-wide queries. Default is current blog. - * @return int Count of inactive users. - */ public static function get_inactive_users_count( $blog_id = null ) { - global $wpdb; - $blog_id = $blog_id ?? get_current_blog_id(); + // Use global cache for network-wide queries (blog_id = 0) + if ( 0 === $blog_id ) { + $cache_key = self::get_inactive_users_count_cache_key( $blog_id ); + $cached_count = wp_cache_get( $cache_key, self::LAST_SEEN_CACHE_GROUP ); - $cache_key = self::get_inactive_users_count_cache_key( $blog_id ); - $cached_count = wp_cache_get( $cache_key, self::LAST_SEEN_CACHE_GROUP ); - - if ( false !== $cached_count ) { - return $cached_count; - } - - $inact_ts = self::get_inactivity_timestamp(); - $release_ts = static::get_last_seen_release_date_timestamp(); - - // Build the inactivity date for user_registered comparison - $inactivity_date = gmdate( 'Y-m-d H:i:s', $inact_ts ); - - // Use WordPress's WP_User_Query to build the capability/role filtering SQL - $capability_query_args = [ - 'fields' => 'ID', - 'count_total' => false, // Prevent SQL_CALC_FOUND_ROWS which doesn't work in subqueries - ]; - - // Add capability or role filtering - if ( ! empty( self::$elevated_capabilities ) ) { - $capability_query_args['capability__in'] = self::$elevated_capabilities; - } else { - $capability_query_args['role__in'] = Capability_Utils::normalize_roles_input( self::$elevated_roles ); - } - - // Get the prepared query data using our utility method - $prepared = Users_Query_Utils::get_prepared_user_query_data( $capability_query_args, $blog_id ); - $user_query = $prepared['query']; - $is_network = $prepared['is_network']; - $capability_where = $prepared['capability_where']; - - // Build the subquery for users with elevated permissions - // Remove SQL_CALC_FOUND_ROWS if present (not compatible with subqueries in MySQL 8+) - $query_fields = str_replace( 'SQL_CALC_FOUND_ROWS ', '', $user_query->query_fields ); - $users_with_caps_subquery = "SELECT DISTINCT {$query_fields} {$user_query->query_from} {$user_query->query_where}"; - - if ( $is_network && ! empty( $capability_where ) ) { - $users_with_caps_subquery .= " AND ({$capability_where})"; - } - - // Build the last_seen conditions - $last_seen_conditions = []; - - // Users with last_seen < inactivity threshold - $last_seen_conditions[] = $wpdb->prepare( '(m_last_seen.meta_value IS NOT NULL AND CAST(m_last_seen.meta_value AS UNSIGNED) < %d)', $inact_ts ); - - // Users without last_seen meta (only if release date is older than cutoff) - if ( $release_ts < $inact_ts ) { - $last_seen_conditions[] = 'm_last_seen.meta_value IS NULL'; + if ( false !== $cached_count ) { + return $cached_count; + } } - $last_seen_where = '(' . implode( ' OR ', $last_seen_conditions ) . ')'; - $last_seen_meta_key = self::LAST_SEEN_META_KEY; - - // include only valid users. - $status_conditions = 'u.user_status = 0'; - if ( is_multisite() ) { - $status_conditions .= ' AND u.spam = 0 AND u.deleted = 0'; - } - - // Build the main SQL query - // phpcs:disable WordPress.DB.PreparedSQL.InterpolatedNotPrepared,WordPressVIPMinimum.Variables.RestrictedVariables.user_meta__wpdb__users - $sql = $wpdb->prepare( - "SELECT COUNT(DISTINCT u.ID) - FROM {$wpdb->users} u - LEFT JOIN {$wpdb->usermeta} m_last_seen - ON u.ID = m_last_seen.user_id - AND m_last_seen.meta_key = %s - WHERE {$status_conditions} - AND u.user_registered < %s - AND {$last_seen_where} - AND u.ID IN ({$users_with_caps_subquery})", - $last_seen_meta_key, - $inactivity_date + /** + * We're doing two separate queries to avoid the query getting too complex and slow since excluding the last_seen meta would add an extra join + * multiplying the complexity of the query beneath it + */ + // Use our utility method that properly handles network-wide capability filtering + $inactive_users_with_last_seen_count = \Automattic\VIP\Security\Utils\Users_Query_Utils::query_users_with_capability_filtering( + self::get_inactive_users_query_args( 'with_last_seen' ), + $blog_id, + true // count only ); - // phpcs:enable WordPress.DB.PreparedSQL.InterpolatedNotPrepared,WordPressVIPMinimum.Variables.RestrictedVariables.user_meta__wpdb__users + $inactive_users_without_last_seen_count = 0; + if ( self::is_release_date_older_than_cutoff() ) { + $inactive_users_without_last_seen_count = \Automattic\VIP\Security\Utils\Users_Query_Utils::query_users_with_capability_filtering( + self::get_inactive_users_query_args( 'without_last_seen' ), + $blog_id, + true // count only + ); + } - // Execute the query - // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared,WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching - $count = (int) $wpdb->get_var( $sql ); + $count = $inactive_users_with_last_seen_count + $inactive_users_without_last_seen_count; // Cache the result for global queries (blog_id = 0) - - $cache_key = self::get_inactive_users_count_cache_key( $blog_id ); - wp_cache_set( $cache_key, $count, self::LAST_SEEN_CACHE_GROUP, self::INACTIVE_USERS_COUNT_CACHE_TTL ); // phpcs:ignore WordPressVIPMinimum.Performance.LowExpiryCacheTime.CacheTimeUndetermined - + if ( 0 === $blog_id ) { + $cache_key = self::get_inactive_users_count_cache_key( $blog_id ); + wp_cache_set( $cache_key, $count, self::LAST_SEEN_CACHE_GROUP, self::INACTIVE_USERS_COUNT_CACHE_TTL ); // phpcs:ignore WordPressVIPMinimum.Performance.LowExpiryCacheTime.CacheTimeUndetermined + } return $count; } diff --git a/tests/phpunit/test-inactive-users.php b/tests/phpunit/test-inactive-users.php index d800bc46..7c17bce4 100644 --- a/tests/phpunit/test-inactive-users.php +++ b/tests/phpunit/test-inactive-users.php @@ -6,8 +6,6 @@ class InactiveUsersTest extends WP_UnitTestCase { private $user_id; private $elevated_roles = [ 'administrator' ]; private $considered_inactive_after_days = 90; - private $additional_user_ids = []; - private $original_release_timestamp; public function setUp(): void { parent::setUp(); @@ -29,56 +27,17 @@ public function setUp(): void { ], ]); } - $this->original_release_timestamp = get_option( Inactive_Users::LAST_SEEN_RELEASE_DATE_TIMESTAMP_OPTION_KEY, false ); // Let's assume the module has been activated today - update_option( Inactive_Users::LAST_SEEN_RELEASE_DATE_TIMESTAMP_OPTION_KEY, time() ); - update_user_meta( $this->user_id, Inactive_Users::LAST_SEEN_META_KEY, time() ); + add_option( Inactive_Users::LAST_SEEN_RELEASE_DATE_TIMESTAMP_OPTION_KEY, time() ); Inactive_Users::init(); } public function tearDown(): void { - foreach ( $this->additional_user_ids as $user_id ) { - wp_delete_user( $user_id ); - } - $this->additional_user_ids = []; - wp_delete_user( $this->user_id ); - - if ( false === $this->original_release_timestamp ) { - delete_option( Inactive_Users::LAST_SEEN_RELEASE_DATE_TIMESTAMP_OPTION_KEY ); - } else { - update_option( Inactive_Users::LAST_SEEN_RELEASE_DATE_TIMESTAMP_OPTION_KEY, $this->original_release_timestamp ); - } - parent::tearDown(); } - /** - * Test that get_inactive_users_count counts users with stale last_seen values. - */ - public function test_get_inactive_users_count_counts_users_with_stale_last_seen(): void { - $baseline_count = $this->get_fresh_inactive_users_count(); - - $user_id = $this->create_test_user(); - update_user_meta( $user_id, Inactive_Users::LAST_SEEN_META_KEY, strtotime( '-91 days' ) ); - - $this->assertSame( $baseline_count + 1, $this->get_fresh_inactive_users_count() ); - } - - /** - * Test that users without last_seen are counted once the release date predates the cutoff. - */ - public function test_get_inactive_users_count_counts_users_without_last_seen_after_release_cutoff(): void { - update_option( Inactive_Users::LAST_SEEN_RELEASE_DATE_TIMESTAMP_OPTION_KEY, strtotime( '-400 days' ) ); - $baseline_count = $this->get_fresh_inactive_users_count(); - - $user_id = $this->create_test_user(); - delete_user_meta( $user_id, Inactive_Users::LAST_SEEN_META_KEY ); - - $this->assertSame( $baseline_count + 1, $this->get_fresh_inactive_users_count() ); - } - /** * Test that add_last_seen_column_head adds a 'Last seen' column to the columns array */ @@ -1148,33 +1107,4 @@ public function test_application_password_authentication_inactive_user() { wp_delete_user( $user_id ); } - - /** - * Create a test user and ensure it is cleaned up after the test. - * - * @param array $overrides Optional. User factory overrides. - * @return int The created user ID. - */ - private function create_test_user( array $overrides = [] ): int { - $defaults = [ - 'role' => 'administrator', - 'user_registered' => gmdate( 'Y-m-d H:i:s', strtotime( '-120 days' ) ), - ]; - - $user_id = $this->factory->user->create( array_merge( $defaults, $overrides ) ); - $this->additional_user_ids[] = $user_id; - - return $user_id; - } - - /** - * Convenience wrapper to fetch a cache-bypassed inactive users count. - * - * @param int|null $blog_id Optional blog ID. - * @return int - */ - private function get_fresh_inactive_users_count( $blog_id = null ): int { - Inactive_Users::flush_cache(); - return Inactive_Users::get_inactive_users_count( $blog_id ); - } } From d47edba5b49c6c0683c8e70aea4c2e6df9f8d8d9 Mon Sep 17 00:00:00 2001 From: Andrea Grassi Date: Mon, 10 Nov 2025 14:10:46 +0100 Subject: [PATCH 8/8] Revert "reintroduce sds hook to sync data" This reverts commit dbdc147e3a65272a81a162780fa109a49b0ea341. --- modules/inactive-users/class-inactive-users.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/inactive-users/class-inactive-users.php b/modules/inactive-users/class-inactive-users.php index 89823cf5..7aa4ee32 100644 --- a/modules/inactive-users/class-inactive-users.php +++ b/modules/inactive-users/class-inactive-users.php @@ -90,8 +90,9 @@ public static function init() { add_action( 'admin_init', [ __CLASS__, 'last_seen_unblock_action' ] ); } + // TODO - Fix performance issues with network-wide queries // Add SDS hook - add_filter( 'vip_site_details_index_security_boost_data', [ __CLASS__, 'add_inactive_users_count_to_sds_payload' ] ); + // add_filter( 'vip_site_details_index_security_boost_data', [ __CLASS__, 'add_inactive_users_count_to_sds_payload' ] ); } public static function maybe_fix_found_users_query() {