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

Prompt for revalidation of 2FA details. #147

Merged
merged 14 commits into from
May 11, 2023
Merged

Prompt for revalidation of 2FA details. #147

merged 14 commits into from
May 11, 2023

Conversation

dd32
Copy link
Member

@dd32 dd32 commented May 4, 2023

Depends upon WordPress/two-factor#529

Fixes #43 #115

Require revalidation of the user via two-factor if accessing two-factor settings or backup codes, and the userRecord.record[ '2fa_revalidation' ].expires_at time has passed.

TODO:

@dd32 dd32 added this to the MVP milestone May 4, 2023
@dd32
Copy link
Member Author

dd32 commented May 4, 2023

The current UI only enforces 2FA revalidation for TOTP and Backup codes, as Email and Password don't need it.

The current code doesn't block a user attempting to setup TOTP or generating backup codes, as while the user could generate a TOTP key and URI, they're unable to save it via the Two Factor plugin (or be able to access the Backup Codes API)

Due to sameorigin policies, using an inline iframe isn't an option, at present, but potentially could be in the future if we wanted to go down that route.

@dd32 dd32 mentioned this pull request May 5, 2023
bazza pushed a commit to WordPress/wordpress.org that referenced this pull request May 8, 2023
@dd32 dd32 force-pushed the add/revalidation branch from 305031b to c9d7003 Compare May 8, 2023 06:02
@dd32
Copy link
Member Author

dd32 commented May 8, 2023

Due to sameorigin policies, using an inline iframe isn't an option, at present, but potentially could be in the future if we wanted to go down that route.

I realised with some minor changes to the upstream patch, and to WordPress.org's SSO, I could make this work.

I have not tested this with a Security key, but due to the Origin being the same, I believe this should work based on some testing using other sites.

Screen.Recording.2023-05-08.at.4.03.58.pm.mov

The delay in the above recording is while it's doing a refreshUser API call, I suspect this can be made much nicer somehow, perhaps just by having the postMessage pass through the new expiry data and have it refresh it in the background, or have it assume that it's okay.

@dd32
Copy link
Member Author

dd32 commented May 9, 2023

With the addition of 3ca07e1 the UI feels much 'faster' and a far better UX.

There's obviously going to be some edge-cases in that, but it appears as if it works well enough.

I haven't tested this PR at all with setting up TOTP, I know it 'works' with the Two-Factor plugin, but I haven't tested our UI with it.

Screen.Recording.2023-05-09.at.11.00.52.am.mov

@dd32
Copy link
Member Author

dd32 commented May 9, 2023

Frustratingly I just realised that this branch appears to be based on an old trunk.. I'll merge/rebase this today. edit: turns out not to have been a problem..

However, I can confirm that the iframe approach works with WebAuthN as in #153

@adamwoodnz adamwoodnz self-assigned this May 9, 2023
@adamwoodnz adamwoodnz marked this pull request as draft May 9, 2023 03:56
@dd32 dd32 marked this pull request as ready for review May 9, 2023 04:56
@dd32 dd32 marked this pull request as draft May 9, 2023 04:56
@adamwoodnz adamwoodnz changed the title Initial work on prompting for revalidation of 2FA details. Promp for revalidation of 2FA details. May 9, 2023
@adamwoodnz
Copy link
Contributor

I'm seeing this warning @dd32, not sure whether it's a concern

Screen Shot 2023-05-10 at 11 40 49 AM

@adamwoodnz
Copy link
Contributor

Latest UI:

  • Single modal flow and copy matching design, but with restyled login page 2FA elements
  • Account Status is displayed behind modal

2FA reauth

Desktop Tablet Mobile
localhost_8000_users_adamwood_edit_account__screen=account-status(Desktop) localhost_8000_users_adamwood_edit_account__screen=account-status(iPad) localhost_8000_users_adamwood_edit_account__screen=account-status(Samsung Galaxy S20 Ultra)

@adamwoodnz adamwoodnz marked this pull request as ready for review May 10, 2023 00:24
@adamwoodnz
Copy link
Contributor

@dd32 there is a translation task in the description but I don't see any other translations in the JS app. What are we expecting here?

@dd32
Copy link
Member Author

dd32 commented May 10, 2023

there is a translation task in the description but I don't see any other translations in the JS app.

Yeah looks like we ditched translations, we can skip that part then for now :)

@dd32
Copy link
Member Author

dd32 commented May 10, 2023

I'm seeing this warning dd32, not sure whether it's a concern

@adamwoodnz Thanks! I'm not sure how I missed that 🤔; I fixed the variable reference, it was a copy-paste error.

Latest UI

Looks good to me! Thanks!

I think we should do some extra work on the 2FA prompts displayed during login too, but that's not needed as part of this, and can wait for the next Iteration.

@adamwoodnz
Copy link
Contributor

Yeah looks like we ditched translations, we can skip that part then for now :)

Cool, guess we're just dependent on the two-factor PR being merged then. Let me know when that happens, or feel free to merge this yourself of course :)

@adamwoodnz adamwoodnz changed the title Promp for revalidation of 2FA details. Prompt for revalidation of 2FA details. May 10, 2023
@StevenDufresne
Copy link
Contributor

I wasn't able to get my environment configured to test this out, but based on what I see, I think this is good solution for the MVP since the grace time should mean that not many users go through this flow (so it's not super critical), but their accounts are protected.

The modal does feel like it has a long chin (a lot of whitespace at the bottom), but I understand it's related to embedding a variable width iframe.

As part of merging this ticket, do ya'll mind opening a new ticket with information about implementing a more ideal case while it's fresh in your mind?

@dd32
Copy link
Member Author

dd32 commented May 11, 2023

As part of merging this ticket, do ya'll mind opening a new ticket with information about implementing a more ideal case while it's fresh in your mind?

I'm not sure what a better solution is really, without too much code duplication.

The only thing I think that could realistically be done here is to improve the actual Prompt screen to not have as much screen reflow (ie. not insert an error div, but rather have it take up some space that is currently visibility: hidden), and have the postMessage communicate the ideal sizes for the embedded iframe.

@dd32 dd32 merged commit a1fa9e5 into trunk May 11, 2023
@dd32 dd32 deleted the add/revalidation branch May 11, 2023 06:51
dd32 added a commit that referenced this pull request May 11, 2023
@adamwoodnz adamwoodnz linked an issue May 16, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Reauth 2FA UI Reauth 2nd factor to change 2FA settings
3 participants