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

Fork upstream WebAuthn provider #134

Closed
wants to merge 6 commits into from
Closed

Fork upstream WebAuthn provider #134

wants to merge 6 commits into from

Conversation

iandunn
Copy link
Member

@iandunn iandunn commented Apr 25, 2023

See #114

This forks the provider from WordPress/two-factor#427 as starting point for our customized version of it. More work will be needed to close that issue, but I'm planning to do it across several PRs to keep it manageable.

This one just forks the upstream PR and gets it working inside our plugin. Future PRs will migrate to the MadWizard library, remove abstractions, increase test coverage, and make other changes to fit w.org's needs. After we refine everything, we can eventually contribute it upstream as an alternate proposal.

A lot of this will be removed or changed significantly in the following PRs, so the only parts that are really worth reviewing are:

  • phpunit.xml.dist
  • wporg-two-factor.php
  • providers/webauthn/class-two-factor-webauthn.php
  • providers/webauthn/webauthn-admin.js
  • providers/webauthn/webauthn-login.js
  • providers/webauthn/webauthn-login.js

@iandunn iandunn added this to the MVP milestone Apr 25, 2023
@iandunn iandunn self-assigned this Apr 25, 2023
This will give us a starting point for our version of the provider. The files were copied verbatim, and then minor changes were made to adjust file paths etc and get it working in this plugin. Future commits will swap in the MadWizard library, and make other changes to fit w.org needs.

Co-Authored-By: mcguffin <[email protected]>
@iandunn iandunn marked this pull request as ready for review April 28, 2023 14:58
@iandunn iandunn requested review from dd32 and pkevan April 28, 2023 14:58
@pkevan
Copy link
Contributor

pkevan commented Apr 28, 2023

Anything in particular we should be looking for or paying attention to. I don't have a suitable device to test with, but unsure if that is necessary?

@iandunn
Copy link
Member Author

iandunn commented Apr 28, 2023

I tested by registering 2 keys, making sure the "test" button worked for each, and that I could log in with each, and then deleting the keys. I also tested the interim login modal.

IMO the most important thing at this phase is identifying any significant code changes that we'll want to make to the provider itself (not its libraries). If you look through the code in the files mentioned above, and note any red flags, that'd be really helpful!

@iandunn
Copy link
Member Author

iandunn commented May 1, 2023

You may be able to use a software key to test with, but I'd recommend expensing a Yubikey or something similar.

Without it, tests will fail incorrectly in local environments.
otherwise get `Uncaught Error: Class "Two_Factor_Provider" not found in wp-content/plugins/wporg-two-factor/providers/webauthn/class-two-factor-webauthn.php:11`
@pkevan
Copy link
Contributor

pkevan commented May 3, 2023

Yubikey or something similar.

Should have one tomorrow, so will be able to test this then.

@iandunn
Copy link
Member Author

iandunn commented May 3, 2023

Moving this back to a draft because integrating this and #146 is turning out to be more time consuming that I hoped. I'm going to try another approach instead.

@iandunn iandunn marked this pull request as draft May 3, 2023 19:50
@pkevan
Copy link
Contributor

pkevan commented May 4, 2023

I didn't test this PR, as it's moved back to draft - had a quick skim of the code and nothing stood out.

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.

2 participants