alleyinteractive / wp-alleyvate

Defaults for WordPress sites by Alley.
GNU General Public License v2.0
16 stars 2 forks source link

force_two_factor_authentication feature causing 502 errors on VIP #95

Open kaitlinbolling opened 1 month ago

kaitlinbolling commented 1 month ago

Description of the bug

On WordPress VIP, when a new user is created via WP CLI or the WordPress admin and two-factor authentication is not enabled, the user experiences a 502 nginx error when trying to log in.

Steps To Reproduce

  1. Create a new user. Do not enable 2fa for this user.
  2. Attempt to log in as the new user, and confirm the 502 error occurs.
  3. Deactivate the Alleyvate plugin.
  4. Attempt to log in as the new user again, and confirm that the 502 error does not occur. Instead, the user is taken to the dashboard and prompted to enable two-factor authentication.

Additional Information

No response

dlh01 commented 4 weeks ago

From what I can tell, this error is caused by an infinite loop:

  1. wpcom_vip_enforce_two_factor_plugin() is called when the VIP two-factor.php plugin loads.
  2. wpcom_vip_two_factor_filter_caps() is added as a filter to map_meta_cap.
  3. wpcom_vip_two_factor_filter_caps() calls wpcom_vip_is_two_factor_forced().
  4. wpcom_vip_is_two_factor_forced() applies the wpcom_vip_is_two_factor_forced filter.
  5. The wpcom_vip_is_two_factor_forced filter calls Force_Two_Factor_Authentication::filter__wpcom_vip_is_two_factor_forced().
  6. Force_Two_Factor_Authentication::filter__wpcom_vip_is_two_factor_forced() calls Force_Two_Factor_Authentication::force_to_enable_2fa().
  7. Force_Two_Factor_Authentication::force_to_enable_2fa() calls current_user_can().
  8. current_user_can() calls map_meta_cap().
  9. map_meta_cap() applies the map_meta_cap filter, which calls wpcom_vip_two_factor_filter_caps() again, etc.

In a local development environment running Alleyvate and the VIP mu-plugins, it should be possible to recreate the infinite loop with:

add_filter( 'wpcom_vip_is_two_factor_local_testing', '__return_true' );

It might be possible to simplify the logic of the Alleyvate 2FA feature when it runs on VIP environments by switching to the wpcom_vip_two_factor_enforcement_cap filter, i.e.:

add_filter( 'wpcom_vip_two_factor_enforcement_cap', fn () => 'edit_posts' );

However, this filter would need to be added before plugins_loaded, which is earlier than Alleyvate currently boots features. The 2FA feature could be handled differently so that it runs earlier, but I think that might be a breaking change that would require documentation and a major version bump.