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

Only show the reauth modal if a primary 2FA provider is enabled #161

Merged
merged 6 commits into from
May 23, 2023

Conversation

adamwoodnz
Copy link
Contributor

Fixes #160

This checks whether the Two_Factor_Totp is available before showing the reauth modal.

In the current state the backup codes are still accessible, but I believe the intention with #47 is to disable access to that screen as well if the Two_Factor_Totp provider is unavailable, see #157

@adamwoodnz adamwoodnz requested a review from dd32 May 12, 2023 03:42
@adamwoodnz adamwoodnz self-assigned this May 12, 2023
@adamwoodnz adamwoodnz changed the title Only show the reauth modal if 2FA by TOTP is enabled Only show the reauth modal if Two_Factor_Totp is enabled May 12, 2023
@adamwoodnz adamwoodnz added this to the MVP milestone May 12, 2023
@adamwoodnz adamwoodnz added bug Something isn't working ui Related to user interface labels May 12, 2023
@pkevan
Copy link
Contributor

pkevan commented May 12, 2023

While this would solve the current setup, I don't think we necessarily want to limit this to just TOTP since we are introducing webauthn too and that would require another fix here - is there any alternative method?

@iandunn
Copy link
Member

iandunn commented May 12, 2023

I don't think we necessarily want to limit this to just TOTP since we are introducing webauthn too and that would require another fix here - is there any alternative method?

#157 introduces a primaryProviderEnabled bool in AccountStatus. If that'd be useful here too, we could maybe move it into script.js, and then add it to GlobalContext so AccountStatus can reuse it.

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 think the root cause for this is actually an upstream bug. I've opened WordPress/two-factor#565 and WordPress/two-factor#566 for that.

I think that will fix this, but haven't tested yet since it's still a WIP.

@adamwoodnz adamwoodnz force-pushed the fix/reauth-when-disabled branch from a719132 to 87c111d Compare May 18, 2023 23:58
@adamwoodnz adamwoodnz force-pushed the fix/reauth-when-disabled branch from 87c111d to 89cb162 Compare May 19, 2023 00:41
Copy link
Member

@dd32 dd32 left a comment

Choose a reason for hiding this comment

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

This works for me, although I've added a few comments on the method of checking, since I think it'd be better to intersect arrays rather than checking for specific keys

@adamwoodnz adamwoodnz marked this pull request as ready for review May 19, 2023 02:58
@adamwoodnz adamwoodnz changed the title Only show the reauth modal if Two_Factor_Totp is enabled Only show the reauth modal if a primary 2FA provider is enabled May 22, 2023
@adamwoodnz adamwoodnz requested a review from iandunn May 22, 2023 04:29
@adamwoodnz adamwoodnz force-pushed the fix/reauth-when-disabled branch from 8f430ee to 6a7290f Compare May 22, 2023 05:21
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.

LGTM 👍🏻

@adamwoodnz adamwoodnz merged commit eb9483d into trunk May 23, 2023
@adamwoodnz adamwoodnz deleted the fix/reauth-when-disabled branch May 23, 2023 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ui Related to user interface
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Fatal in reauth modal when 2FA is disabled
5 participants