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

Reauth 2FA UI #115

Closed
renintw opened this issue Apr 19, 2023 · 13 comments · Fixed by #147
Closed

Reauth 2FA UI #115

renintw opened this issue Apr 19, 2023 · 13 comments · Fixed by #147
Assignees
Labels
needs design priority: high ui Related to user interface
Milestone

Comments

@renintw
Copy link
Contributor

renintw commented Apr 19, 2023

Ref: #43 (comment)

Acceptance Criteria

Prompt user when it's been > xx mins since the last 2FA challenge before displaying UI.

  • Question: Done by the back-end side? If not, we might need to Store the timestamp of the last 2FA challenge: When the user successfully completes a 2FA challenge, store the timestamp (e.g., as a Unix timestamp), such as storing in a cookie, local storage or Recat state.
@renintw renintw added ui Related to user interface needs design priority: high labels Apr 19, 2023
@renintw renintw added this to the MVP milestone Apr 19, 2023
@renintw renintw changed the title UI for Reauth 2FA before changing any 2FA settings UI for 2FA Reauth Apr 19, 2023
@renintw renintw changed the title UI for 2FA Reauth Reauth 2FA UI Apr 19, 2023
@iandunn
Copy link
Member

iandunn commented Apr 19, 2023

I think we should also deny any API POST requests, to avoid those being made manually. Some of that may happen automatically because of the upstream plugin, but we should make sure that our custom API endpoint don't allow a bypass.

@StevenDufresne
Copy link
Contributor

Thanks for creating this, is it worth creating some screenshots and making some quick wireframes/flows of how/where this will work?

@StevenDufresne
Copy link
Contributor

StevenDufresne commented Apr 25, 2023

Here's a quick attempt I made which definitely needs a @WordPress/meta-design opinion and tuning. But I think we can probably do it with a overlay. Thoughts?

Screenshot

This would require that we know that the session has expired when users try to update values. I'm thinking we could write a middleware that handles those updates.

@StevenDufresne StevenDufresne modified the milestones: MVP, Iteration 1 Apr 25, 2023
@jasmussen
Copy link
Collaborator

Modal generally looks good. On a meta note, we are updating the modal in @wordpress/components, not sure if those are used here, but it'd be nice to sync up the language. That means in general a 32px padding inside, and an 8px border radius. Here's a quick sketch:

Modal

I think my main question here is the larger flow, can you walk me through which things I had to click to get here? This is when you go back to 2fa to change it, correct?

@StevenDufresne
Copy link
Contributor

I think my main question here is the larger flow, can you walk me through which things I had to click to get here? This is when you go back to 2fa to change it, correct?

You can think about this as a session expiry scenario. Here is what we expect to happen:

  1. User logs in with 2fa configured (or recently configured)
  2. User can update email, password, generate backups codes, essentially all 2fa features
  3. User leaves there computer for 16 minutes (2fa session expires)
  4. User tries to update email or password or generated backups
  5. We present the Reauth UI (the ui we are working on here)
  6. Users enters codes and clicks the "Update" button ("Enable" doesn't make sense for the modal)
  7. Values are updated

What this is doing is protecting a scenario where a 2fa session is left open and unattended.

@jasmussen
Copy link
Collaborator

Sounds good. If the modal is the most basic way to unstick this, sounds good to move forward. This is an interface we'll continue to QA and even visually refresh at some point, if anything sticks out we'll come back to it.

@renintw
Copy link
Contributor Author

renintw commented Apr 28, 2023

Note: Extra components that would be helpful.

  1. Extra support for users losing their devices. For example, the backup code option at the bottom.
  2. Error prompt.

@renintw
Copy link
Contributor Author

renintw commented Apr 28, 2023

On a meta note, we are updating the modal in @wordpress/components, not sure if those are used here, but it'd be nice to sync up the language

Tried building it using @wordpress/components, doesn't seem to have any big issues.

image

@StevenDufresne
Copy link
Contributor

Tried building it using @wordpress/components, doesn't seem to have any big issues.

It may makes sense to use that component but be sensitive to loading Gutenberg libraries (or any third party libraries) and its effect on page size. From what I remember the component library used to cost about 1 MB.

@StevenDufresne
Copy link
Contributor

@renintw Can we a draft PR that mimics the behavior ahead of the upstream PR so we can try it?

@renintw
Copy link
Contributor Author

renintw commented May 2, 2023

@renintw Can we a draft PR that mimics the behavior ahead of the upstream PR so we can try it?

yeah, no problem. That's what I had in mind. 👍

@dd32
Copy link
Member

dd32 commented May 5, 2023

Added some initial work in #147 for this, as I hadn't seen this issue prior.

While the UI's in this issue look good, they're not really viable to be implemented as part of the MVP IMHO.

This is in part due to technical limitations around the 2FA providers in use, and the browser security modals we have to work within. There's also very little benefit for us to create a new front-end UI for displaying the re-auth flow for TOTP, as we'd then need to integrate with WebAuthN as well, which would likely require significant JS duplication from the login flow.

For now, the best way forward is going to be to redirect them off to the login screen I think, but we can (and should) iterate on the 2FA challenge screen both during login and re-auth, either for WordPress.org specifically or in combination with the Two-Factor plugin.

Previously the Two-Factor plugin was not very extensible in this regard, and replacing it's UI was not super straight forward. That being said, it can now be replaced wholesale by adding a authentication_page() method to https://github.com/WordPress/wporg-two-factor/blob/trunk/class-encrypted-totp-provider.php which replaces/duplicates https://github.com/WordPress/two-factor/blob/master/providers/class-two-factor-totp.php#L654-L682 and something similar for WebAuthN.

That being said, I'm going to investigate whether we can display it within an inline iframe in a modal, as it'll provide a much better experience. Iframes aren't going to work here. It would have to be a duplicated UI, and for WebAuthN that would mean something similar to all the JS under https://github.com/sjinks/wp-two-factor-provider-webauthn/blob/d82e030647da1d811a5911095314d3ee7cc000c9/inc/class-webauthn-provider.php#L52-L95

@dd32
Copy link
Member

dd32 commented May 8, 2023

Iframes aren't going to work here.

Good news, I realised over the weekend that I could indeed make this work. #147 has been updated to use an iframe, and is much more user friendly now.

#147 (comment)

@dd32 dd32 modified the milestones: Iteration 1, MVP May 15, 2023
@adamwoodnz adamwoodnz linked a pull request May 16, 2023 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs design priority: high ui Related to user interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants