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

AUT-3921: Change content for mfa reset with IPV #2444

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alhcomer
Copy link
Contributor

@alhcomer alhcomer commented Jan 7, 2025

What

Made content changes for MFA screens to reflect that MFA relies on approval from IPV.

SMS MFA:
Screenshot 2025-01-15 at 17 02 41

AUTH APP MFA:
Screenshot 2025-01-15 at 17 01 33

Changes to the acceptance tests have been made to reflect these changes. They are commented out currently, to be uncommented when MFA_RESET_WITH_IPV is turned on.

Happy for this to be merged in my absence if no changes need to be made.

How to review

  1. Code Review
  2. Go through sign in journey up to MFA page:
  • Check that for SMS you see: 'If you no longer have access to this phone number, check if you can change how you get security codes.'

  • Check that for AUTH_APP you see: 'If you no longer have access to your authenticator app, check if you can change how you get security codes.'

Checklist

  • Performance analyst has been notified of the change.
  • A UCD review has been performed.
  • Documentation has been updated to reflect these changes.

Related PRs

@alhcomer alhcomer requested review from a team as code owners January 7, 2025 12:22
@alhcomer alhcomer marked this pull request as draft January 7, 2025 12:22
@alhcomer alhcomer force-pushed the AUT-3921/mfa-with-ipv-content-change branch 7 times, most recently from 8a3299e to aec4645 Compare January 14, 2025 08:58
@alhcomer alhcomer force-pushed the AUT-3921/mfa-with-ipv-content-change branch from aec4645 to 243a907 Compare January 14, 2025 14:24
@alhcomer alhcomer marked this pull request as ready for review January 15, 2025 17:11
@alhcomer alhcomer changed the title AUT-3982: Change content for sms mfa reset with IPV AUT-3921: Change content for sms mfa reset with IPV Jan 15, 2025
@alhcomer alhcomer changed the title AUT-3921: Change content for sms mfa reset with IPV AUT-3921: Change content for mfa reset with IPV Jan 15, 2025
Copy link
Contributor

@BeckaL BeckaL left a comment

Choose a reason for hiding this comment

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

Do you also need to apply these changes to the auth app uplift template?

<a href={{ mfaResetPath }} class="govuk-link"
rel="noreferrer noopener">{{ 'pages.enterMfa.details.changeGetSecurityCodesLinkText'| translate }}</a>.
</p>
{% if supportAccountRecovery === true or supportAccountRecovery === "true" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a repeated line

rel="noreferrer noopener">{{ 'pages.enterMfa.details.changeGetSecurityCodesLinkText'| translate }}</a>.
</p>
{% if supportAccountRecovery === true or supportAccountRecovery === "true" %}
{% if supportMfaResetWithIpv === true or supportMfaResetWithIpv === "true" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you need to add this as a hidden input also - does the wrong thing appear if you refresh the page?

Copy link
Contributor

@BeckaL BeckaL Jan 16, 2025

Choose a reason for hiding this comment

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

Just did a quick test and doesn't seem to make a difference, which makes me wonder if we're doing an amount of this cacheing unnecessarily. Worth verifying on your side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants