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

Suport 2FA-Plugin #76

Closed
ouun opened this issue Oct 25, 2021 · 11 comments · Fixed by #83
Closed

Suport 2FA-Plugin #76

ouun opened this issue Oct 25, 2021 · 11 comments · Fixed by #83
Assignees
Milestone

Comments

@ouun
Copy link
Contributor

ouun commented Oct 25, 2021

@JJJ this is a very useful plugin, thanks a lot!
I would like to suggest that it supports the 2FA-Plugin (https://wordpress.org/plugins/two-factor/) by default.
Currently the settings land in the "Others" tab and it does not work as expected due JS issues.

Kind regards,

Philipp

@JJJ
Copy link
Contributor

JJJ commented Oct 26, 2021

Thank you @ouun for the heads up. I will take a look at this soon.

Two-factor auth is very important, and supporting it directly & natively in this plugin makes sense. 👍

@JJJ JJJ self-assigned this Oct 26, 2021
@JJJ JJJ added this to the 3.0 milestone Oct 26, 2021
@ouun
Copy link
Contributor Author

ouun commented Aug 15, 2022

Hi @JJJ,
I hope you are doing great!
I don't want to generate any pressure. But I'd be very grateful for your estimate of when this could be implemented. Do you believe it would be possible this year?

Thanks and kind regards,

Philipp

@JJJ
Copy link
Contributor

JJJ commented Aug 19, 2022

@ouun this year, yes!

I do hope to give this plugin some attention in the coming weeks. 💟

@carstingaxion
Copy link
Contributor

I gave this combination a try today, knowing about this particular issue.

During my investigation I found some obsolete usage of check_admin_referrer(), not here but, at the other side.

I wasn't able to find any not working stuff at all. Even my console doesn't showed any errors, when updating one of the 2FA options or when I used the Generate Verification Codes Button.

Could you please describe what you expected @ouun and what happened and where? ;)

@carstingaxion
Copy link
Contributor

Ok, one thing, but it's not related to JS:

The 2FA Plugin gernerates an admin_notice with a link of /wp-admin/profile.php#two-factor-backup-codes, when you've no backup-codes left within your user-account.

In combination with wp-user-profiles, this link will go to /wp-admin/users.php?page=profile#two-factor-backup-codes which is not the needed page|tab of Other, where the settings are located. Unfortunately, I ad hoc had no idea on a nice solution yet.

@ouun
Copy link
Contributor Author

ouun commented Feb 19, 2023

Hi @carstingaxion,
Thank you very much for getting back to me. It is quite a while passed since I tested the combination. Should have more more specific at that time. However tested it again with a fresh WP instance:

I guess the JS issue I mentioned is that the "Register New Key"-Button for FIDO-Keys does not work when WP User Profiles is active.

The other improvement suggestion was to extract 2FA-Settings from the "Others"-Tab. Maybe "Acount" or a "Security"-Tab would be a more intuitive position.

Firefox Developer Edition - ‹ WordPress Test Drive — WordPress 2023-02-19 um 16 03 14

Thanks and kind rergards,

Philipp

@carstingaxion
Copy link
Contributor

Ola @ouun,

thank you for taking the time.

I guess the JS issue I mentioned is that the "Register New Key"-Button for FIDO-Keys does not work when WP User Profiles is active.

You're right. I'll have a look into it.

The other improvement suggestion was to extract 2FA-Settings from the "Others"-Tab. Maybe "Acount" or a "Security"-Tab would be a more intuitive position.

Exactly what I had in mind, too. But didn't wrote it down.

@carstingaxion
Copy link
Contributor

carstingaxion commented Feb 21, 2023

Fiddling with

the "Register New Key"-Button for FIDO-Keys

I was able to bring back the required javascripts from the Two Factor Plugin, by just enqueing them.

// Better compatibility for two-factor
// 
// Part1 : Bring back .js
add_action( 'admin_enqueue_scripts', function ( string $hook ) {
	
	if ('users_page_other' !== $hook )
		return;

	if ( ! class_exists('Two_Factor_FIDO_U2F_Admin')) {
		require_once TWO_FACTOR_DIR . 'providers/class-two-factor-fido-u2f-admin.php';
	}
	
	// this is missing, due to 'wp-user-profiles' different $hook global
	// luckily, we can just call it
	Two_Factor_FIDO_U2F_Admin::enqueue_assets( 'profile.php' );
}, 
// two-factor registers on 5 and enqueues at 10
0 );

For development puposes just loaded from a mu-plugins file for the moment.

But now, comes Part 2:

Failed to execute 'postMessage' on 'DOMWindow': The target origin provided ('chrome-extension://...') does not match the recipient window's origin ('null')

And this one I get even without wp-user-profiles enabled, as you can see from the window title of the first screenshot.

Error while using the two-factor Plugin, without using the wp-user-profiles plugin

Error while using the two-factor Plugin AND using the wp-user-profiles plugin

No ideas, yet.

WebAuthn/FIDO2 is being added in WordPress/two-factor#427 , and the existing FIDO1 keys may be migrated (see WordPress/two-factor#439). Those are both scheduled for the 0.8.0 release.

Right now we have 55% of the 0.8.0 milestone.

@carstingaxion
Copy link
Contributor

carstingaxion commented Feb 21, 2023

Going on with Part 3 : Move 2FA into account tab

remove_action( 'show_user_profile', array( 'Two_Factor_Core', 'user_two_factor_options' ) );
remove_action( 'edit_user_profile', array( 'Two_Factor_Core', 'user_two_factor_options' ) );
	
// fast & dirty, without own metabox
add_action( 'wp_user_profiles_password_metabox_after', array( 'Two_Factor_Core', 'user_two_factor_options' ) );

Screenshot 2023-02-21 11 57 36

Works.

@carstingaxion
Copy link
Contributor

Ola @JJJ . Where would be the best place to hold such compatibility code?

I would like open a PR with a cleaned and polished implementation.

@JJJ
Copy link
Contributor

JJJ commented Feb 23, 2023

This is looking awesome so far!

@carstingaxion I think, use your best judgement! 😅 Since Two-Factor is a core WordPress initiative, treat it as if it were already merged and ready? We can conditionally hook/unhook it based on any criteria later (versions, activations, etc...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants