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

Disable U2F Interface unless already configured. #571

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

dd32
Copy link
Member

@dd32 dd32 commented May 25, 2023

What?

This PR disables the U2F / Fido interface, unless keys are already configured for the user.

Fixes #511

Why?

U2F / FIDO no longer works in modern browsers, until #423 is resolved having this provider enabled only causes confusion to end users (See #511)

Ideally, we wouldn't need to do this, as we've been assuming that #423 would be resolved, but 6+ months later it's no closer to being merged. I'd like to merge this into a 0.8.2.

Alternatives

Alternatively, the Javascript could be updated to detect FIDO/U2F not being viable, and displaying an error message about the browser not supporting it too..

How?

This simply disables the UI by:

  • Removing it from the Providers array when disabling the table, if the provider says it's not available.
  • Returning early when displaying the Security keys table, if the provider says it's not available.
  • Returning early when enqueuing assets when no keys are registered.

If for some reason, it needs to be re-enabled a filter is included:

add_filter( 'two_factor_u2f_disabled', '__return_false' );

Testing Instructions

Screenshots or screencast

Before After
Screenshot 2023-05-25 at 6 12 46 pm Screenshot 2023-05-25 at 6 13 04 pm

Changelog Entry

Deprecated: The FIDO/U2F integration has been hidden unless already configured. This is because modern browsers no longer support the standard, and we've not yet finalised our WebAuthn implementation.

@dd32 dd32 requested a review from jeffpaul May 25, 2023 08:17
@dd32 dd32 added the FIDO U2F label May 25, 2023
Copy link

@ravinderk ravinderk left a comment

Choose a reason for hiding this comment

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

@dd32 This pull request is working fine. I left a minor request.

class-two-factor-core.php Outdated Show resolved Hide resolved
@jeffpaul
Copy link
Member

jeffpaul commented Jun 9, 2023

@dd32 do you want to pull this into 0.8.2 or leave for 0.9.0?

@jeffpaul jeffpaul added this to the 0.10.0 milestone May 8, 2024
@jeffpaul
Copy link
Member

Besides the merge conflicts @georgestephanis @TimothyBJacobs does this look good to you?

@TimothyBJacobs
Copy link
Member

Yeah I think this makes sense to me.

@TimothyBJacobs TimothyBJacobs mentioned this pull request Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FIDO U2F Security Keys not being enabled
4 participants