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

Use the Rest API for setting up individual providers #504

Merged
merged 2 commits into from
Feb 6, 2023

Conversation

dd32
Copy link
Member

@dd32 dd32 commented Dec 8, 2022

This draft PR is some work at extending the plugin to allow for alternate UI interfaces to the plugin.

It does this by using a REST API endpoint for the primary actions in TOTP and Backup Codes.

This is still tightly-coupled to the rest of the provider class, as it still expects the HTML returned by ::user_two_factor_options() is intended on being displayed.

Due to the nature of this change, there appears to be a lot of churn, but that's mostly wholesale replacement of code.

This PR does not yet have unit test coverage, failures are to be expected.
This is intended to make WordPress/wporg-two-factor#18 / WordPress/wporg-two-factor#22 easier.

@pkevan
Copy link
Contributor

pkevan commented Dec 13, 2022

Tested and both backup codes and TOTP work - the only slight complication (and not directly related to this PR) is the introduction of the TOTP JS via #487 meaning you have to build and compile in order to use it, perhaps we should consider committing the JS file for qrcode generator into the repo so this step isn't necessary - it seems like the only thing we are building that isn't part of the development tools (unit tests etc)

@iandunn
Copy link
Member

iandunn commented Dec 13, 2022

Installing it via npm is important IMO so that we get notifications of security vulns in the package. We could still leave it in dependencies but then manually commit it to the repo, though 🤔

I'm somewhat hesitant to disconnect them, because that introduces the opportunity for them to get out of sync (which could disguise any potential security issues if it weren't noticed). I'm open to it if there's a way to document it clearly; or even better if there's a way to automatically enforce the sync (especially for the built version).

@dd32
Copy link
Member Author

dd32 commented Dec 14, 2022

This is pretty off-topic, but.. While I don't necessarily see using npm for the JS library important for security purposes, I don't see the build step as a blocker either.
This is a development repo, not one designed to be used in production.
You also builds it as part of the local-environment setup.

In the future, it seems likely that there'll be more dependencies introduced for WebAuthn #427 that would benefit from installation through dependencies.

@iandunn
Copy link
Member

iandunn commented Jan 11, 2023

I pushed a few commits that are needed for WordPress/wporg-two-factor#30:

  • 541d87c makes the function reusable by other plugins, and also easier to grok/maintain since it's now only doing "one thing".
  • b478cd7 is needed to enable the TOTP provider when POST /totp is called to set it up. Previously the secret was saved in usermeta, but the provider was still disabled. That affected both this plugin and wporg-two-factor. The Backup Codes provider may need something similar, but I haven't checked. There may also be a better way to integrate it with Two_Factor_Core::user_two_factor_options_update(), but I didn't see a good one.

@iandunn
Copy link
Member

iandunn commented Jan 25, 2023

I added two more commits:

  • 1e347aa fixes a bug in b478cd7 where the providers would get duplicated inside _two_factor_enabled_providers.
  • 726d372 enables the Backup Codes provider when the codes are generated, similar to what b478cd7 does for TOTP

@iandunn
Copy link
Member

iandunn commented Jan 26, 2023

895a5c2 adds an enabled_provider param to the setup routes. b478cd7 and 726d372 unintentionally changed the wp-admin behavior by automatically enabling the provider. That's only desired in the custom UI, though.

It's off by default because the wp-admin UI has a separate fields for enabling the provider, and those are set when the user clicks Update Profile.

Custom UIs might want to automatically enable it, though, rather than having to submit a 2nd request.

iandunn added a commit to WordPress/wporg-two-factor that referenced this pull request Jan 26, 2023
This is explicitly needed now because of a recent change in the upstream pull request.
See WordPress/two-factor#504 (comment)
Copy link
Member

@iandunn iandunn left a comment

Choose a reason for hiding this comment

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

I've been testing this with wporg-two-factor's custom UI and it's working well. I also tested the native wp-admin UI and it worked too.

I think it'd still be good to get @kasparsd's feedback before merging, though, in case he has any thoughts from the perspective of maintaining this plugin.

providers/class-two-factor-backup-codes.php Outdated Show resolved Hide resolved
providers/class-two-factor-backup-codes.php Outdated Show resolved Hide resolved
public function register_rest_routes() {
register_rest_route(
Two_Factor_Core::REST_NAMESPACE,
'/totp',
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be ideal to have more explicit route names, rather than distinguishing them by the method. e.g., two-factor/1.0/totp/enable and two-factor/1.0/totp/disable.

Not a blocker though, just a preference.

tests/providers/class-two-factor-backup-codes-rest-api.php Outdated Show resolved Hide resolved
@iandunn
Copy link
Member

iandunn commented Feb 6, 2023

I'm going go go ahead and merge this since it's been sitting for awhile, but we can definitely iterate if there's any more feedback.

@iandunn iandunn merged commit 3aa9c84 into WordPress:master Feb 6, 2023
iandunn added a commit that referenced this pull request Feb 8, 2023
New tests were added for `rest_delete_totp` in #504 which cover the same functionality.

`generate_qr_code_url` was also refactored there, but a test wasn't included.
@iandunn
Copy link
Member

iandunn commented Feb 9, 2023

If anyone comes here looking for commits 3aa9c84 (TOTP) or 8baec51 (Backup Codes), I changed their hashes to be5c0a9 and 4d3cb4f, respectively.

Before merging I reset this branch and created those two earlier commits, so they'd be atomic for master. After merging, I realized I'd forgotten to set the author to Dion, who did most of the work here. I didn't want to take credit for that, and the commits had only been in master for ~18 hours, so I went ahead and fixed the metadata and force-pushed master. The contents of the commits are the same, though.

r-a-y added a commit to r-a-y/bp-two-factor that referenced this pull request Apr 26, 2023
In two-factor v0.8, a bunch of class methods and constants were
removed as part of the overhaul to use the REST API instead of AJAX.
See WordPress/two-factor#504.

This broke our custom, frontend implementation handler. More to come.

See #1.
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.

4 participants