fix: clean up two factor #26901
fix: clean up two factor #26901
Conversation
zlwaterfield
requested review from
benjackwhite,
raquelmsmith,
patricio-posthog and
surbhi-posthog
|
Size Change: 0 B Total Size: 1.11 MB ℹ️ View Unchanged
|
|
I see failing tests, so I'm just going to re-run them to see if they pass |
| actions.loadStatus() | ||
|
|
||
| if (values.user && values.user.organization?.enforce_2fa && !values.user.is_2fa_enabled) { | ||
| actions.openTwoFactorSetupModal(true) |
There was a problem hiding this comment.
I feel like this is confusing because the modal could be already open (if it's forced open), yet we're saying here to open the modal. but instead of it opening the modal, what's actually happening is this side-effect where startSetup is called.
I think it technically works but it's confusing and took me a bit to understand the code.
There was a problem hiding this comment.
On load, this will check the user's status and open the modal if the user doesn't have 2fa but is required to have it. This will only be called when the application is first loaded because it's rendered in global modals. The modal can't already be opened because this will be called before the UI renders a button to open it manually. This is the only place that force opens the modal.
There was a problem hiding this comment.
Curious - what is confusing about it? Do you have thoughts on a different way it could be done?
|
Tested locally for the three cases and everything looks good. I'm just trying to figure out what's going on with these failing tests so I'm re-basing and rerunning them since they seem unrelated to these changes. |
|
Test recording |
|
It will pass locally but still need get the requests stubbed or else CI won't pass. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Changes
There's currently a race condition introduced here https://github.com/PostHog/posthog/pull/26813/files that may double trigger the set up call and make it so the QR code being rendered doesn't match the token stored in the session.
This PR also cleans up the set up code so the modal is only rendered once and not in 3 locations.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
It doesn't have an impact.
How did you test this code?
Manually.
I'd love for the review to pull this locally and test all 3 cases to make sure it's all working