[HOLD for payment 2024-03-25] [$500] Web - 2FA - Authentication code value is not accepted when put Space and than correct it

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.37-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers): applausetester+0507gm@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

Precondition:
User should have 2-FA enabled

1.Log in to staging.new.expensify.com
2. Enter email address from precondition
3. Navigate to email and locate the magic link - change to staging if needed
4. Open an new tab and navigate to the staging link
5. Verify the 2FA required page is displayed
6. Navigate back to the original tab
7. Click on "Use recovery code" to change to "Use 2 factor authentication code"
8. Enter authentication code with space at the beginning
9. Click Sign in
10. Error message will appear
11. Erase the space
12. Click Sign in

Expected Result:

Error should dismiss and user should successfully log in

Actual Result:

Error message reappears and user can not log in

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6369712_1707245790430.Recording__2113.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018d55723e0e20585e
• Upwork Job ID: 1754943906473668608
• Last Price Increase: 2024-02-06
• Automatic offers:
• getusha | Reviewer | 0
• neonbhai | Contributor | 0

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018d55723e0e20585e

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @getusha (External)

[melvin-bot]

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[allgandalf]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Authentication code value is not accepted when put Space and than correct it

What is the root cause of that problem?

We don't have sanity checks for the input value in the textbox

App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js

Lines 286 to 289 in 8563c0f

	if (!ValidationUtils.isValidRecoveryCode(recoveryCode)) {
	setFormError({recoveryCode: 'recoveryCodeForm.error.incorrectRecoveryCode'});
	return;
	}

Also the max length of textbox is 8,

What changes do you think we should make in order to solve the problem?

have a sanity check to remove all the whitespaces and pass clean value to validate. Also while sending data to the backend below, trim the input to remove whitespaces

App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js

Line 306 in 8563c0f

const recoveryCodeOr2faCode = props.isUsingRecoveryCode ? recoveryCode : twoFactorAuthCode;

Update the isValidRecoveryCode check to:

if (!ValidationUtils.isValidRecoveryCode(recoveryCode.trim())) { 

and update the submit input code to:

 const recoveryCodeOr2faCode = props.isUsingRecoveryCode ? recoveryCode.trim() : twoFactorAuthCode; 

Update the RECOVERY_CODE_LENGTH to 10 for considering one extra space before and after the string

App/src/CONST.ts

Line 934 in f6ba751

RECOVERY_CODE_LENGTH: 8,

This will help us revalidate our logic as if we remove the maxLength, then the user will be allowed to enter the value and push it for validation on the backend, we should not let it call to the BE and set the error there itself on the FE

Problem with removing maxLength

Here we will still send the whitespaces if we remove the maxLength and which will return an error, so trimming is still required

App/src/libs/ValidationUtils.ts

Lines 284 to 286 in f6ba751

	function isValidRecoveryCode(recoveryCode: string): boolean {
	return Boolean(recoveryCode.match(CONST.RECOVERY_CODE_REGEX_STRING));
	}

[muas19]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Authentication code value is not accepted when put Space and than correct it

What is the root cause of that problem?

The validation check in this file does not cater for spaces

App/src/libs/ValidationUtils.ts

Line 285 in 8563c0f

return Boolean(recoveryCode.match(CONST.RECOVERY_CODE_REGEX_STRING));

What changes do you think we should make in order to solve the problem?

We should update the REGEX pattern in the const file for spaces or trim it.

App/src/CONST.ts

Line 883 in 8563c0f

RECOVERY_CODE_REGEX_STRING: /^[a-zA-Z0-9]{8}$/,

What alternative solutions did you explore? (Optional)

na

[allgandalf]

Proposal updated

Only updated the Solution, no concept change only added code to support my solution

[neonbhai]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Authentication code value is not accepted when put Space and than correct it.

What is the root cause of that problem?

This happened as when pasting with space the last digit did not get pasted.

This is a UX bug as we do not expect the recovery code input to have a tight character limit (or trim silently when pasting).

It is also unclear that the recovery code limit is 8 (since it is not a standard number).

We should remove the maxLength prop here

What changes do you think we should make in order to solve the problem?

We will remove the maxLength prop from the Recovery Code Text Input here

Alternatively

We can set it to FORM_CHARACTER_LIMIT, like we do for some form elements

[getusha]

@GandalfGwaihir's proposal Looks good to me, trimming the value makes sense.
🎀 👀 🎀 C+ Reviewed!

[melvin-bot]

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[neonbhai]

@getusha hi, seems like the original error happened has we have a tight maxLength (so the whole code didn't get pasted). We can see this in the video that the j at the end of the code didn't get pasted. The issue is still reproducible after trimming the values.

Requesting you to take another look 🙇

[allgandalf]

Proposal updated

Considering the comments from @neonbhai , i have proposed updated solution, but have proposed a different approach and we still need to trim the input as we cannot allow it to be passed to the BE, also removing maxLength is not a good idea instead i propose we update the value to 10 considering a whitespace before and after the desired string, if we remove maxLength then the user would be able to add as many length of string as he wishes to which is not desired

[getusha]

thanks for pointing this out @neonbhai, will re-evaluate proposals.

[allgandalf]

hello @getusha , my updated proposal will also be reviewed in the re-evaluation process right?

[neonbhai]

Yes, but you still seem to have an incorrect RCA

[allgandalf]

I think the RCA is going to be a mix of both our approaches as even when you remove the maxLength, it is still going to throw an error

[getusha]

@neonbhai after removing maxLength how can we ensure we allow the right length for the input.

[neonbhai]

I think we can keep it optimistic like FORM_CHARACTER_LIMIT. So it has some editing space but not infinite, like we do for other inputs in app.

Similar issue here did not keep the editing space very tight, even though the actual length of input would be 3-5

[allgandalf]

We would still require to trim the input as the original bug issue wanted to get logged in even if there were spaces but the code entered is correct

[getusha]

thanks everyone! The proposal from @neonbhai looks good to me the RCA is correct and the solution works. it would be great if we could have a condition to check the trimmed length of the value and set it accordingly in onTextInput if possible @neonbhai

🎀 👀 🎀 C+ Reviewed!

[melvin-bot]

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

📣 @getusha 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[puneetlath]

@neonbhai how's it going?

[neonbhai]

Raising PR shortly!

[getusha]

@neonbhai any issues i can help with?

[neonbhai]

Having trouble with the android build, updating screenshots shortly!

[melvin-bot]

This issue has not been updated in over 15 days. @puneetlath, @neonbhai, @getusha eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[neonbhai]

PR has been merged and deployed to staging

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.53-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Recovery code input fix #36774

If no regressions arise, payment will be issued on 2024-03-25. 🎊

For reference, here are some details about the assignees on this issue:

• @neonbhai requires payment automatic offer (Contributor)
• @getusha requires payment automatic offer (Reviewer)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@getusha] The PR that introduced the bug has been identified. Link to the PR:
• [@getusha] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@getusha] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@getusha] Determine if we should create a regression test for this bug.
• [@getusha] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[puneetlath]

@getusha friendly reminder about the checklist so that we can pay on Monday.

[puneetlath]

@neonbhai has been paid. Still waiting on the checklist for @getusha

[getusha]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@getusha] The PR that introduced the bug has been identified. Link to the PR: Added recovery code option to 2fa #23390
• [@getusha] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/23390/files#r1537753571
• [@getusha] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/a
• [@getusha] Determine if we should create a regression test for this bug. No, seems like a very minor bug and i don't think we need we need a regression test for it.
• [@getusha] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again. N/a

[getusha]

@puneetlath apologies, I forgot about the checklist despite your reminder. 😄

[puneetlath]

No worries! Thanks for taking care of it.

It looks like it didn't auto-offer you for some reason. Offer is here: https://www.upwork.com/nx/wm/offer/101569210

Please ping me on the issue when you've accepted

[getusha]

@puneetlath accepted.

[puneetlath]

All paid. Thanks everyone!