UI: allow retries for MFA form errors #27574

noelledaley · 2024-06-21T22:21:50Z

🛠️ Description

Fixes a bug where the MFA form wasn't displaying an error message nor countdown when a user tried to enter an incorrect one-time password too many times.
Fixes JIRA ticket # VAULT-28437

📸 Screenshots

Before (notice missing error banner)

After (error banner & countdown shown, button disabled)

🏗️ How to Build and Test the Change

To replicate this you'll need to set up MFA and a userpass user. I recommend having two browser windows open: one for the root user and one for a userpass user.

Create a policy test with all privileges (so the user can eventually enable MFA on their login):

path "*" {
  capabilities = ["create", "read", "update", "delete", "list", "sudo"]
}

Enable the userpass auth method
Create a user with the test policy
In the left sidebar, click "Multi-factor authentication"
Select TOTP and 'next'
Fill out Vault as the issuer (though it doesn't really matter). Enter 2 as the max validation attempts. Under Enforcements select "skip this step".
Click create.
Copy the MFA id from the URL. Ex 60c04ac9-943b-79f6-f466-e1f1d2c2fe19 in mfa/methods/60c04ac9-943b-79f6-f466-e1f1d2c2fe19
In an incognito window, go login as the user you created in step 3.
Once you're logged in, click "MFA" from the user dropdown. Paste in the MFA id from step 8.
Save the QR code in 1password as a one-time-password.
Back in your root user window, create a new enforcement.
Enter a name (is can be anything), and select TOTP. from the MFA methods search. Under targets, select "userpass". Click add to add it to the list of targets.
Now, back in your incognito, log out and try to log in again as your userpass user. You should be taken to the MFA form.
Enter an incorrect OTP. You should see an error.
Enter an incorrect OTP 3 more times. You should see the countdown, an error indicating that you have tried too many times and must wait, and the button should be disabled.
The error should disappear after the countdown is finished.
Entering a correct OTP should allow you to log in.

TODO only if you're a HashiCorp employee

Labels: If this PR is the CE portion of an ENT change, and that ENT change is
getting backported to N-2, use the new style backport/ent/x.x.x+ent labels
instead of the old style backport/x.x.x labels.
Labels: If this PR is a CE only change, it can only be backported to N, so use
the normal backport/x.x.x label (there should be only 1).
ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
Jira: If this change has an associated Jira, it's referenced either
in the PR description, commit message, or branch name.
RFC: If this change has an associated RFC, please link it in the description.
ENT PR: If this change has an associated ENT PR, please link it in the
description. Also, make sure the changelog is in this PR, not in your ENT PR.

github-actions · 2024-06-21T22:32:39Z

CI Results:
All Go tests succeeded! ✅

github-actions · 2024-06-21T23:20:06Z

Build Results:
All builds succeeded! ✅

hellobontempo

Incredibly diligence QA-ing and finding this bug! Just a question about whether the test passed or failed before when values ended in 0. (Mostly for curiosity sake)

hellobontempo · 2024-06-21T23:56:25Z

ui/tests/integration/components/mfa-form-test.js

@@ -173,9 +173,11 @@ module('Integration | Component | mfa-form', function (hooks) {
 // TODO JLR: It doesn't appear that cancelTimers is working and tests wait for the full countdown
 test('it should show countdown on passcode already used and rate limit errors', async function (assert) {
 const messages = {
- used: 'code already used; new code is available in 45 seconds',
+ used: 'code already used; new code is available in 30 seconds',
+ // there an intentional typo in the error message (30s seconds)


Thank you for this comment! "Typo" is a little ambiguous. I wonder if it's worth explicitly saying "the backend returns duplicate duration s" or something to make it clearer what the typo is?

Also, were you able to verify if this test failed or passed with the prior code when the seconds didn't end in 0? If so, it may be worth mentioning that somewhere that the regex didn't account for times ended in 0

yep, the tests failed once i used the old regex. and i added a comment about the backend returning a duplicate string, thanks!

noelledaley · 2024-06-22T00:10:52Z

ui/app/components/mfa/mfa-form.js

+ const delayRegExMatches = errorMessage.match(/(\d+\w seconds)/);
+ if (delayRegExMatches) {
+ delay = delayRegExMatches[0].split(' ')[0];
+ } else {
+ // default to 30 seconds if no delay is found
+ delay = 30;
+ }
+ this.countdown = parseInt(delay);


fixing this ding-dang regex was what resolved the bug 🙃

i also opted to add a safe default of 30s in case the API error message changes again too. it'll only apply if the error message contains the phrases from lines 89-90 above though, which will prevent us from adding a timeout if the API errors for an unrelated reason.

noelledaley · 2024-06-22T00:11:10Z

ui/tests/integration/components/mfa-form-test.js

 assert.dom('[data-test-mfa-validate]').isDisabled('Button is disabled during countdown');
 assert.dom('[data-test-mfa-passcode]').isDisabled('Input is disabled during countdown');
 assert.dom('[data-test-inline-error-message]').exists('Alert message renders');
 }
 });

+ test('it defaults countdown to 30 seconds if error message does not indicate when user can try again ', async function (assert) {


test coverage for the default timeout that i added

hellobontempo · 2024-06-24T16:28:30Z

ui/app/components/mfa/mfa-form.js

 // parse validity period from error string to initialize countdown
- this.countdown = parseInt(message.match(/(\d\w seconds)/)[0].split(' ')[0]);
+ const delayRegExMatches = errorMessage.match(/(\d+\w seconds)/);


Great idea wrapping this in a conditional!

hellobontempo · 2024-06-24T16:32:03Z

ui/app/components/mfa/mfa-form.js

+ if (delayRegExMatches) {
+ delay = delayRegExMatches[0].split(' ')[0];
+ } else {
+ // default to 30 seconds if no delay is found


This also means we'll set a default if the parsed integer is 0. 🤔 I think this is okay...(I'm not sure a 0s period is even possible) Maybe adding to the comment here to remind future devs that this else block will also be hit if the delay is zero?

fair point, i'll update the conditional to if (delayRegExMatches && delayRegExMatches.length) to make that more explicit 😎

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jun 21, 2024

noelledaley changed the title ~~mfa-form: fix regex matching so error msg displays~~ UI: allow retries for MFA form errors Jun 21, 2024

noelledaley added this to the 1.17.1 milestone Jun 21, 2024

noelledaley added the ui label Jun 21, 2024

noelledaley added backport/1.17.x backport/ent/1.16.x+ent Changes are backported to 1.16.x+ent labels Jun 21, 2024

noelledaley force-pushed the ui/fix-mfa-reattempts branch from 456306a to 8e8ad5c Compare June 21, 2024 23:14

noelledaley marked this pull request as ready for review June 21, 2024 23:14

noelledaley requested a review from a team as a code owner June 21, 2024 23:14

hellobontempo approved these changes Jun 22, 2024

View reviewed changes

noelledaley force-pushed the ui/fix-mfa-reattempts branch from 8e8ad5c to d06bea2 Compare June 22, 2024 00:06

noelledaley commented Jun 22, 2024

View reviewed changes

hellobontempo reviewed Jun 24, 2024

View reviewed changes

noelledaley modified the milestones: 1.17.1, 1.17.2 Jun 24, 2024

vercel bot deployed to Preview June 25, 2024 17:54 View deployment

noelledaley added 2 commits June 25, 2024 16:25

mfa-form: fix regex matching so error msg displays

9bb6cfa

changelog

522dddb

noelledaley force-pushed the ui/fix-mfa-reattempts branch from 31d61fa to 29a98ae Compare June 25, 2024 23:27

chore: add comments

3b57f9c

noelledaley force-pushed the ui/fix-mfa-reattempts branch from 29a98ae to 3b57f9c Compare June 25, 2024 23:32

noelledaley enabled auto-merge (squash) June 25, 2024 23:38

noelledaley merged commit aa828f1 into main Jun 25, 2024
31 checks passed

noelledaley deleted the ui/fix-mfa-reattempts branch June 25, 2024 23:49

hc-github-team-secure-vault-core mentioned this pull request Jun 25, 2024

Backport of UI: allow retries for MFA form errors into release/1.17.x #27607

Merged

6 tasks

noelledaley added the backport/ent/1.15.x+ent Changes are backported to 1.15.x+ent label Jun 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: allow retries for MFA form errors #27574

UI: allow retries for MFA form errors #27574

noelledaley commented Jun 21, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024 •

edited

Loading

hellobontempo left a comment

hellobontempo Jun 21, 2024

noelledaley Jun 25, 2024

noelledaley Jun 22, 2024

noelledaley Jun 22, 2024

hellobontempo Jun 24, 2024

hellobontempo Jun 24, 2024

noelledaley Jun 25, 2024 •

edited

Loading

UI: allow retries for MFA form errors #27574

UI: allow retries for MFA form errors #27574

Conversation

noelledaley commented Jun 21, 2024 • edited Loading

🛠️ Description

📸 Screenshots

Before (notice missing error banner)

After (error banner & countdown shown, button disabled)

🏗️ How to Build and Test the Change

TODO only if you're a HashiCorp employee

github-actions bot commented Jun 21, 2024 • edited Loading

github-actions bot commented Jun 21, 2024 • edited Loading

hellobontempo left a comment

Choose a reason for hiding this comment

hellobontempo Jun 21, 2024

Choose a reason for hiding this comment

noelledaley Jun 25, 2024

Choose a reason for hiding this comment

noelledaley Jun 22, 2024

Choose a reason for hiding this comment

noelledaley Jun 22, 2024

Choose a reason for hiding this comment

hellobontempo Jun 24, 2024

Choose a reason for hiding this comment

hellobontempo Jun 24, 2024

Choose a reason for hiding this comment

noelledaley Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

noelledaley commented Jun 21, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024 •

edited

Loading

noelledaley Jun 25, 2024 •

edited

Loading