Pared Down Core Proposal: Two Factor Authentication.

[georgestephanis]

This pull request is to pare down the functionality in the Two Factor plugin, to a base amount that would make it more feasible for merge into WordPress Core.

With just the Email and potentially Backup Codes methods, there's far less configuration required, and as such less opportunity for users to mistakenly lock themselves out of their accounts.

All remaining methods -- TOTP, U2F, etc -- could be considered either at a later date, or left to the purview of plugins.

[pwaring]

Would this mean the end of the plugin supporting U2F and TOTP, or would that functionality continue separately to core? I use this plugin on at least 10 sites, so if it’s going to cease working after a merge into core then I’d need to find an alternative.

[voldemortensen]

I would like to propose TOTP be kept as a stronger method. Email is a weaker form of MFA. To activate TOTP a token provider must be set up and a token entered. This greatly reduces the likelihood of someone being locked out. I support U2F being pushed back. Browser and general site support is still lacking.

…

On Nov 9, 2019, at 11:51, George Stephanis ***@***.***> wrote:  This pull request is to pare down the functionality in the Two Factor plugin, to a base amount that would make it more feasible for merge into WordPress Core. With just the Email and potentially Backup Codes methods, there's far less configuration required, and as such less opportunity for users to mistakenly lock themselves out of their accounts. All remaining methods -- TOTP, U2F, etc -- could be considered either at a later date, or left to the purview of plugins. You can view, comment on, or merge this pull request online at: #306 Commit Summary Remove U2F, TOTP, Dummy methods for Two Factor functionality. For the initial core proposal, let's keep it simple to E-Mail and backup codes. Update the url paths reflecting the move from georgestephanis's github account to the WordPress organizational account. File Changes M class.two-factor-core.php (13) M composer.json (2) D includes/Google/u2f-api.js (748) D includes/Yubico/U2F.php (507) D providers/class.two-factor-dummy.php (93) D providers/class.two-factor-fido-u2f-admin-list-table.php (152) D providers/class.two-factor-fido-u2f-admin.php (341) D providers/class.two-factor-fido-u2f.php (384) D providers/class.two-factor-totp.php (509) D providers/css/fido-u2f-admin.css (10) D providers/js/fido-u2f-admin-inline-edit.js (145) D providers/js/fido-u2f-admin.js (48) D providers/js/fido-u2f-login.js (16) M readme.txt (7) D tests/providers/class.two-factor-dummy.php (88) D tests/providers/class.two-factor-fido-u2f.php (198) D tests/providers/class.two-factor-totp.php (267) M two-factor.php (4) Patch Links: https://github.com/WordPress/two-factor/pull/306.patch https://github.com/WordPress/two-factor/pull/306.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[kasparsd]

@georgestephanis Great to know that there is renewed interest in getting this in core. Starting with just a minimum of features sounds like a great approach!

Considering that this plugin is currently active on 10k+ sites (including all VIP Go sites as a mu-plugin), I would strongly recommend that we create a new feature plugin for WP core to avoid breaking security for sites that rely on the current feature set of this plugin.

This would also allow us to improve the codebase -- get rid of singletons, re-organize the files to support providers into separate directories with their own CSS and JS, etc.

[georgestephanis]

Right! So to answer some concerns --

This PR is never intended to be merged into the main branch. Ever. It's just here so there's an easier-to-comment-on place for the merge branch.

My hope is that whatever functionality doesn't merge into Core (U2F, TOTP, ???) remains in the two-factor plugin, so folks who have the two-factor plugin will not see any loss of functionality. Kinda how the Gutenberg plugin kept adding new blocks after core functionality was merged to core.

My only concern about TOTP is that we'd need some sort of library to generate the QR Codes for folks to scan into Google Authenticator / Authy / 1Password / etc. I'd rather not continue to depend on Google hosted apis to generate the QR Code in core.

If we do include TOTP -- and I've had this discussion with @Ipstenu and others back at the community summit in Philly years ago -- we need to make email permanently defaulted on as a fallback, as otherwise there are going to be a lot of admins who use Google Authenticator without a backup method, put their phone through the laundry machine (or drop it in a toilet, lose it, etc) and then get locked out of their site because they don't know how to FTP, having only installed WordPress through a host's one-click install. If someone opts to disable email as a fallback, we need to make it very clear that they may be permanently locked out unless they can log into the server to fix it.

(as an ancillary note, that's why I've been advocating for @authy for years, having been locked out of some accounts the first time I lost a phone and the totp keys that I was using google authenticator on)

[Otto42]

Including TOTP is an absolute minimum for 2FA. It's simple, the code is public and open, and generating QR codes isn't that hard if we need to include it.

[orenwolf]

If we do include TOTP -- and I've had this discussion with @Ipstenu and others back at the community summit in Philly years ago -- we need to make email permanently defaulted on as a fallback

This exists today as a filter:

add_filter(
    'two_factor_enabled_providers_for_user',
    function( $providers ) {
        if ( empty( $providers ) && class_exists( 'Two_Factor_Email' ) ) {
            $providers[] = 'Two_Factor_Email';
        }

        return $providers;
    }
);

[voldemortensen]

If we do include TOTP -- and I've had this discussion with @Ipstenu and others back at the community summit in Philly years ago -- we need to make email permanently defaulted on as a fallback

Forcing email as a fallback defeats the purpose of adding TOTP as a more secure method of 2FA. I would rather display a warning if someone only has a single 2FA method enabled, not viewed their backup codes, or something similar.

My only concern about TOTP is that we'd need some sort of library to generate the QR Codes for folks to scan into Google Authenticator / Authy / 1Password / etc. I'd rather not continue to depend on Google hosted apis to generate the QR Code in core.

I agree with this. With core's recent PHP version bumps I'm sure we can find a library suitable for our needs. If not, I am willing to write one if it means I can have TOTP 2FA.

[orenwolf]

Forcing email as a fallback defeats the purpose of adding TOTP as a more secure method of 2FA. I would rather display a warning if someone only has a single 2FA method enabled, not viewed their backup codes, or something similar.

The filter as setup today does not "force" email - it enables email if no other option is set. This means the default is for it to be enabled, but any user can choose to disable it if they wish, they just need to select any other option, and explicitly disable email in their profile.

[jeffpaul]

@georgestephanis given the age of this PR and the advancement of the plugin since then, I wonder if:

1. there is work in this PR that's good to pull into the plugin itself?
2. this PR is otherwise best to close out given its age
3. if we might scrub through the open issues/PRs to identify what else is ideal to land in the plugin such that we can work on a refreshed core merge proposal?