Optionally Force 2fa

[mikeselander]

Fixes #255.

Adds a network-level and site level (if non-multisite) setting to require two-factor authentication on a per-role or global level. If 2fa forcing is on and a user matches the criteria, we force the user into a 2fa settings screen and they cannot use the site until they add valid 2fa to their account.

Super administrators can edit the settings with this interface via network (or site if single site) settings:

Users who fall within the required buckets will see an interface like this:

[mikeselander]

Hey folks, apologies - this is nowhere near review ready and I hit the wrong button 🤦‍♂️

[kasparsd]

@mikeselander This looks great! Will you open a new pull request later or should I re-open this one?

[mikeselander]

@kasparsd it still needs quite a bit of smoothing out which I'll be working on today. I'll re-open this once it's in a better state :)

[iandunn]

🤔 Wouldn't removing the user's capabilities solve that (assuming the plugin is written correctly)? Or are you thinking of something different?

[joehoyle]

I've fixed the merge conflicts here!

[joshbetz]

Maybe this is obvious, but removing caps only serves to motivate people to enable Two Factor. It doesn't enhance security because anyone with the username and password can just enable 2FA to get full access. If you have a privileged user that never enables 2FA (maybe they don't login for a long time), their account is still vulnerable via a pwned password.

I would suggest an alternative way to think about this might be to enable Email by default for anyone that doesn't have other factors and then require everyone to have at least 1 factor enabled. (So you could disable email after enable TOTP, for example.)

I know this gets complicated if you want to enable Email as a factor, so maybe the default factor could be configurable. For example, maybe you want an admin to be able to generate a backup code and deliver it out of band.

[joshbetz]

Could be as simple as:

	add_filter( 'two_factor_enabled_providers_for_user', 'force_2fa', 10, 2 );
	function force_2fa( $enabled, $user_id ) {
		if ( ! $force_2fa ) {
			return $enabled;
		}
		
		if ( count( $enabled ) ) {
			return $enabled;
		}

		return [ 'Two_Factor_Email' ];
	}

[NewJenk]

Could be as simple as:

	add_filter( 'two_factor_enabled_providers_for_user', 'force_2fa', 10, 2 );
	function force_2fa( $enabled, $user_id ) {
		if ( ! $force_2fa ) {
			return $enabled;
		}
		
		if ( count( $enabled ) ) {
			return $enabled;
		}

		return [ 'Two_Factor_Email' ];
	}

I think there's a minor typo here, guessing $force_2fa should be $user_id, like this:

add_filter( 'two_factor_enabled_providers_for_user', 'force_2fa', 10, 2 );
function force_2fa( $enabled, $user_id ) {
	if ( ! $user_id ) {
		return $enabled;
	}

	if ( count( $enabled ) ) {
		return $enabled;
	}

	return [ 'Two_Factor_Email' ];
}

I've quickly tested this locally and it seems to work great - a really nice way to quickly and conveniently enforce email 2fa. It also automatically pre-selects email on the user settings screen, so if a user wants to change 2fa settings they'll see that email is pre-selected.

[alexclst]

There should be a way to hard-code the force settings from wp-config.php, so that we can enforce these in a way that normal admin users cannot change. While filters are more capable, I think that also having a define we can add would further harden security. Otherwise one admin could get 2FA set up, and then disable the requirement right away.

[jeffpaul]

@iandunn @joehoyle @georgestephanis is there a preferred approach to either getting this updated and towards merge (or perhaps closed with a fresh PR and new approach and then towards merge)? If so, I can try and help pull some engineering time to get this to done.

[eric-michel]

We've been using the "use email if nothing selected" method of forcing 2FA:

function force_2fa_for_users($providers, $user_id) {
    // Force Email provider if no providers are enabled
    if (empty($providers) && class_exists('Two_Factor_Email')) {
      $providers[] = 'Two_Factor_Email';
    }
    
    return $providers;
  }

This works quite well (although I recently found that if the user selected FIDO U2F but enters no keys, 2FA gets disabled) Ultimately, I think an official "require 2FA feature" could be barely more complicated than the function I've shown above. Adding a filter to turn it on + the ability for a developer to set a wp-config setting and forcing email as a fallback method when no other is available is fully sufficient.

I'd go so far as to say requiring 2FA should be on by default with the option to disable that feature, but I understand not wanting to change the behavior for existing installations.

[masteradhoc]

This feature has been merged inside the Humanmade Fork for quite some time and seems in use there. Good to recheck and see if we can merge this. See humanmade#1

[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.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @mikeselander, @tcrsavage, @alternativesurfer, @adambirds, @naomicbush, @Onyx808, @brianjohnpenner, @NewJenk.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

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

Unlinked contributors: mikeselander, tcrsavage, alternativesurfer, adambirds, naomicbush, Onyx808, brianjohnpenner, NewJenk.

Co-authored-by: joehoyle <joehoyle@git.wordpress.org>
Co-authored-by: shadyvb <shadyvb@git.wordpress.org>
Co-authored-by: mikelittle <mikelittle@git.wordpress.org>
Co-authored-by: kasparsd <kasparsd@git.wordpress.org>
Co-authored-by: rmccue <rmccue@git.wordpress.org>
Co-authored-by: BrookeDot <brookedot@git.wordpress.org>
Co-authored-by: flowdee <flowdee@git.wordpress.org>
Co-authored-by: jeffpaul <jeffpaul@git.wordpress.org>
Co-authored-by: alexclst <alexclst@git.wordpress.org>
Co-authored-by: iandunn <iandunn@git.wordpress.org>
Co-authored-by: joshbetz <betzster@git.wordpress.org>
Co-authored-by: eric-michel <ytfeldrawkcab@git.wordpress.org>
Co-authored-by: masteradhoc <masteradhoc@git.wordpress.org>
Co-authored-by: apermo <apermo@git.wordpress.org>

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

[BrookeDot]

@masteradhoc Thanks for pointing to that fork. I took a quick look so may have missed it, but does the HM fork include a filter to enable/disable this functionality?

My quick read shows that it's enabled by default. While in the case of this feature plugin it should likely be a simple opt-in. My general preference would be with a filters for minimum capability and enabled/disabled, but also I can see the value of having it in the UI.

In either case, I think a revisit of #255 and a fresh PR is the way to go. I would like to see agreement on:

• Filters and WP-config option or just filters (I vote filters only)
• Enabled or disabled by default? (I vote disabled)
• Include UI (I vote yes)

@jeffpaul as you were the most recent comment in regards to a fresh PR. and @mikeselander as you opened the original PR even if by your own admission it was supposed to be as a draft PR.

If we can get some consensus I can take a look at likely forking the Human Made approach back into core but will likely need some help writing the tests.

[masteradhoc]

@BrookeDot your help would be very appreciated! I think as well that we want to:

• give admins in the settings page an option to force it for user roles of their choice
• filter to activate/disable this behaviour is already there as far as i saw but good to doublecheck
• constant over wp config is for sure a great idea

So far my opinion :)

[BrookeDot]

filter to activate/disable this behaviour is already there as far as i saw but good to doublecheck

🤦‍♀️ The file was callapsed because it was new, but does contain the main logic with filters:
https://github.com/humanmade/two-factor/blob/252e3e0891ecf08adaf910d1ea5debd20ad39170/class.two-factor-force.php

Looking at:

/**
	 * Get which user roles have two-factor forced.
	 *
	 * @since 0.1-dev
	 *
	 * @return array
	 */
	public static function get_forced_user_roles() {
		/**
		 * User roles which have two-factor forced.
		 *
		 * @param array $roles Roles which are required to use 2fa.
		 */
		return (array) apply_filters( 'two_factor_forced_user_roles', get_site_option( self::FORCED_ROLES_META_KEY, false ) );
	}

Brings up the question of if this should be role based (easier to understand for the average user) or if it should be cap based (simplified implementation, as one could just do edit_posts for example instead of selecting multiple roles)

[masteradhoc]

@BrookeDot i think user roles would be quite easier to understand from a UX point.