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

UI: allow retries for MFA form errors #27574

Merged
merged 3 commits into from
Jun 25, 2024
Merged

UI: allow retries for MFA form errors #27574

merged 3 commits into from
Jun 25, 2024

Conversation

andaley
Copy link
Contributor

@andaley andaley commented Jun 21, 2024

🛠️ 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)

Screenshot 2024-06-21 at 3 57 30 PM

After (error banner & countdown shown, button disabled)

Screenshot 2024-06-21 at 3 55 36 PM

🏗️ 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.

  1. 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"]
}
  1. Enable the userpass auth method
  2. Create a user with the test policy
  3. In the left sidebar, click "Multi-factor authentication"
  4. Select TOTP and 'next'
  5. 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".
  6. Click create.
  7. Copy the MFA id from the URL. Ex 60c04ac9-943b-79f6-f466-e1f1d2c2fe19 in mfa/methods/60c04ac9-943b-79f6-f466-e1f1d2c2fe19
  8. In an incognito window, go login as the user you created in step 3.
  9. Once you're logged in, click "MFA" from the user dropdown. Paste in the MFA id from step 8.
  10. Save the QR code in 1password as a one-time-password.
  11. Back in your root user window, create a new enforcement.
  12. 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.
  13. 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.
  14. Enter an incorrect OTP. You should see an error.
  15. 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.
  16. The error should disappear after the countdown is finished.
  17. 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 github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jun 21, 2024
@andaley andaley changed the title mfa-form: fix regex matching so error msg displays UI: allow retries for MFA form errors Jun 21, 2024
@andaley andaley added this to the 1.17.1 milestone Jun 21, 2024
@andaley andaley added the ui label Jun 21, 2024
Copy link

github-actions bot commented Jun 21, 2024

CI Results:
All Go tests succeeded! ✅

@andaley andaley added backport/1.17.x backport/ent/1.16.x+ent Changes are backported to 1.16.x+ent labels Jun 21, 2024
@andaley andaley force-pushed the ui/fix-mfa-reattempts branch from 456306a to 8e8ad5c Compare June 21, 2024 23:14
@andaley andaley marked this pull request as ready for review June 21, 2024 23:14
@andaley andaley requested a review from a team as a code owner June 21, 2024 23:14
Copy link

github-actions bot commented Jun 21, 2024

Build Results:
All builds succeeded! ✅

Copy link
Contributor

@hellobontempo hellobontempo left a comment

Choose a reason for hiding this comment

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

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)

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@andaley andaley force-pushed the ui/fix-mfa-reattempts branch from 8e8ad5c to d06bea2 Compare June 22, 2024 00:06
Comment on lines 109 to 116
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test coverage for the default timeout that i added

// 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)/);
Copy link
Contributor

Choose a reason for hiding this comment

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

Great idea wrapping this in a conditional!

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@andaley andaley Jun 25, 2024

Choose a reason for hiding this comment

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

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

@andaley andaley force-pushed the ui/fix-mfa-reattempts branch from 31d61fa to 29a98ae Compare June 25, 2024 23:27
@andaley andaley force-pushed the ui/fix-mfa-reattempts branch from 29a98ae to 3b57f9c Compare June 25, 2024 23:32
@andaley andaley enabled auto-merge (squash) June 25, 2024 23:38
@andaley andaley merged commit aa828f1 into main Jun 25, 2024
31 checks passed
@andaley andaley deleted the ui/fix-mfa-reattempts branch June 25, 2024 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/ent/1.16.x+ent Changes are backported to 1.16.x+ent hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants