Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Apr 12, 2023

Conversation

dd32
Copy link
Member

@dd32 dd32 commented Mar 22, 2023

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:

  • Filter addition
  • Removing variables like 'class' and replacing them with 'provider_key'
  • Addition of a Two_Factor_Provider::get_key() which returns the providers 'key', which defaults to the class name as present.
  • removing get_instance() from each individual provider and moving it to the abstract Two_Factor_Provider, each provider had a slightly different one, and the usage of __CLASS__ meant children of the provider had to override the get_instance() method as well.

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

  • Unit tests: npm run test
  • Add the above code into an mu-plugin, verify that you can setup TOTP and login, but once code is removed, the key is invalid.

Screenshots or screencast

Changelog Entry

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

class-two-factor-core.php Outdated Show resolved Hide resolved
@dd32 dd32 marked this pull request as draft March 22, 2023 06:41
@dd32 dd32 marked this pull request as ready for review March 22, 2023 06:52
@jeffpaul jeffpaul added this to the 0.9.0 milestone Mar 22, 2023
Copy link
Contributor

@pkevan pkevan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@dd32
Copy link
Member Author

dd32 commented Apr 5, 2023

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

@dd32 dd32 merged commit c490519 into WordPress:master Apr 12, 2023
@dd32 dd32 deleted the filter/provider-class branch April 12, 2023 05:35
dd32 added a commit to dd32/two-factor that referenced this pull request Apr 13, 2023
dd32 added a commit that referenced this pull request Apr 19, 2023
* Store the two factor login timestamp and provider in the user session.

* Create the user session directly attaching data to it, rather than using filters.

* Add a method to determine if the current login session is two factored.

* Use an anonymous function attached to a callback to set the user session information.

* Tests: Add a test that validates that is_current_user_session_two_factor() doesn't return true when the cookie is set from outside of the two-factor handler.

* Make is_current_user_session_two_factor() more readable.

* Make Two_Factor_Core::login_form_validate_2fa() testable, by not calling exit;

* Tests: Add tests that validate that the 2fa status is appropriately set in the user session.

* Remove misplaced `@covers` annotation.

Co-authored-by: Ian Dunn <ian@iandunn.name>

* Correct an @Covers annotation.

Co-authored-by: Ian Dunn <ian@iandunn.name>

* Update to reflect #546

* Tests: Simplify the cookie management.

* Be more explicit about the return value.

* Use a static anonymous function, as $this isn't needed.

* Simplify the wrapper by allowing _login_form_validate_2fa() to always be exited after calling.

* Simplify by passing $provider as a string always.

* Update @SInCE tags.

---------

Co-authored-by: Ian Dunn <ian@iandunn.name>
@kasparsd kasparsd mentioned this pull request Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants