WordPress / two-factor

Two-Factor Authentication for WordPress.
https://wordpress.org/plugins/two-factor/
GNU General Public License v2.0
731 stars 153 forks source link

Add a filter to filter the classname used for a provider #546

Closed dd32 closed 1 year ago

dd32 commented 1 year ago

What?

This PR allows for the classname of a provider to differ from it's "key" that's used within the application. This was spurred by an attempt to simply extend a core provider with a custom one to override certain functions (instead of #545 ).

This has resulted in several changes:

Why?

This allows for users of the plugin to overload a provider with a custom variant of the provider, without having to register an entirely new provider and migrate users over to it.

For example:


add_filter( 'two_factor_provider_classname_Two_Factor_Totp', function( $provider ) {
    return 'Secured_Two_Factor_Totp';
} );

// Stores TOTP keys securely rot13'd.
class Secured_Two_Factor_Totp extends Two_Factor_Totp {
    // Pretend to be the Two_Factor_Totp class in the UI.
    public function get_key() {
        return 'Two_Factor_Totp';
    }

    // When saving the key, ensure it's rot13'd
    public function set_user_totp_key( $user_id, $key ) {
        $key = str_rot13( $key );
        return parent::set_user_totp_key( $user_id, $key );
    }

    // When fetching the key, rot13 it back.
    public function get_user_totp_key( $user_id ) {
        return str_rot13( parent::get_user_totp_key( $user_id ) );
    }
}

How?

By allowing the classname to be filtered, external code can overload core providers with customisations without needing to duplicate the entire provider.

Testing Instructions

Screenshots or screencast

Changelog Entry

Added - Better compatibility to allow overloading core Two Factor providers without duplicating the entire provider.

dd32 commented 1 year ago

Code looks good - is there an example we can test where someone has extended the providers?

The best example is the unit test included, and the code included in the PR description.

My only concern with the changes to class instantiation might conflict with someone who has previously set this up.

That's a valid concern, however, it's pretty awkward to extend them at present to the point that this shouldn't break any existing implementations of that. They'd have to be including their own replacement class in the two_factor_providers array, and as it's currently keyed by the classname, couldn't "transparently replace" a core provider without modifying code, AFAIK.

I also can't find any plugins in the directory that extend the core providers: https://wpdirectory.net/search/01GX87VBAWAPCCA0RXEY3HY7G0