WordPress / wporg-two-factor

2FA for WordPress.org accounts
38 stars 8 forks source link

Download backup codes to file #22

Closed pkevan closed 1 year ago

pkevan commented 1 year ago

Make sure a user can download the backup codes easily, and it able to confirm they have done so (or written them down).

pkevan commented 1 year ago

Related: #15

pkevan commented 1 year ago

See: https://github.com/pkevan/two-factor/commit/0f6ee66d3c2136a430d2d37655b432795f70748e

iandunn commented 1 year ago

i'm just prototyping the functionality and following pre-existing patterns in the plugin.

How are you thinking we'd use the prototype? Is that something you plan to merge and build on for the MVP milestone, or just something you were playing around with to get a rough feel for things before starting on a React/Gutenberg based PR?

pkevan commented 1 year ago

Was mainly thinking as simple as possible, and then push a more complicated UI to later.

I guess part of the problem is diverging too much from the core plugin.

iandunn commented 1 year ago

as simple as possible, and then push a more complicated UI to later.

I could see the benefit of that if it was a situation where we would iterate our way to our desired goal. In this case, though, it seems like we'd be starting with a fundamentally different architecture, and would have to mostly rewrite it, which I doubt we'd ever do realistically. If we did do that, it'd require much more work than if we used the desired architecture from the start.

diverging too much from the core plugin

We've already decided to build a custom UI, so I don't understand what the benefit of sticking close to the upstream plugin is in this case. How would the UI in plugins/two-factor/providers/class.two-factor-backup-codes.php integrate into the WPCOM-based UI we're building in wporg-two-factor ?

pkevan commented 1 year ago

OK, makes sense. I think I got somewhat confused between making sure that some of the efforts can be pushed back upstream and our intentions. Thanks for clarifying that.

iandunn commented 1 year ago

Ah, I see what you mean 👍🏻 I was thinking that we were planning to build a new UI from scratch, and then maybe in the future we'd contribute parts of it upstream, but that wasn't a priority for the MVP. I know we've all talked about different ideas at different times and in different mediums, though, and I'm not sure we ever explicitly made a decision about any of it.

I definitely have concerns about maintaining a custom UI in the long term, but I got the impression that it was the only way to really achieve the design and UX/flow that we want (at least without a lot of hacks).

@dd32 , @tellyworth does that line up with what you were thinking? It'll help to make sure we're all on the same page

dd32 commented 1 year ago

I was thinking that we were planning to build a new UI from scratch

This was my understanding. That we'd just create the UI from scratch, ignoring the upstream UI for now.

There are some things that would make sense to contribute upstream immediately, such as migrating some things to a rest api instead of profile action handlers, etc. If required for our UI.

https://github.com/pkevan/two-factor/commit/0f6ee66d3c2136a430d2d37655b432795f70748e

I was thinking we could do this purely client-side, make the XHR request, get the codes and download link, display the codes / download link, Click Next button, hide the codes and button and then ask for one of the codes to be entered, validate it client-side (Since we really only need to validate the user-action at this point, not record it) and then make the XHR or page reload that actually fully turns on 2FA.

dd32 commented 1 year ago

There are some things that would make sense to contribute upstream immediately, such as migrating some things to a rest api instead of profile action handlers, etc. If required for our UI.

For that, which might make our UI easier, I converted the existing Backup Codes & TOTP UI's to use a rest api: https://github.com/WordPress/two-factor/pull/504

iandunn commented 1 year ago

Closing this since we have dedicated issues for the client-side UI now: #39 and #40