[HOLD for payment 2023-10-02] [$1000] Redesign 2FA input prompt to allow recovery codes input

[MonilBhavsar]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Coming from this customer's issue https://github.com/Expensify/Expensify/issues/295936

Action Performed:

Break down in numbered steps

1. Enable 2FA in Newdot
2. Download recovery codes
3. Assuming one loses access to authenticator app

Expected Result:

Describe what you think should've happened
User should be able to login by entering their recovery codes

Actual Result:

Describe what actually happened
User is unable to enter recovery codes.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Signin into olddot, renable 2FA and then come to newdot.
Confusing from user's perspective

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Screen.Recording.2023-07-05.at.7.03.39.PM.mov

Version Number:
Reproducible in staging?:
Reproducible in production?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e2527e617e6edb31
• Upwork Job ID: 1678281063937363968
2023-07-17
• Automatic offers:
• abdulrahuman5196 | Reviewer | 25671030
• alitoshmatov | Contributor | 25671031

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[MonilBhavsar]

@shawnborton I think we can use a simple text input we have for login, right?

[shawnborton]

Exactly, we already have a simple text input component we could use.

[laurenreidexpensify]

@MonilBhavsar anything else required here before or can I add external?

[MonilBhavsar]

Yes, we can move ahead 👍

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01e2527e617e6edb31

[melvin-bot]

Current assignee @laurenreidexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[Vishrut19]

Is this issue already assigned to someone ?

[alitoshmatov]

Proposal

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

Redesign 2FA input prompt to allow recovery codes input

What is the root cause of that problem?

This is feature request

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

We can add checkbox or text link here saying like Use recovery code and when pressed we switch to normal text input from magic code input

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

Lines 244 to 259 in c51349b

	<View style={[styles.mv3]}>
	<MagicCodeInput
	autoComplete={props.autoComplete}
	ref={input2FARef}
	label={props.translate('common.twoFactorCode')}
	name="twoFactorAuthCode"
	value={twoFactorAuthCode}
	onChangeText={(text) => onTextInput(text, 'twoFactorAuthCode')}
	onFulfill={validateAndSubmitForm}
	maxLength={CONST.TFA_CODE_LENGTH}
	errorText={formError.twoFactorAuthCode ? props.translate(formError.twoFactorAuthCode) : ''}
	hasError={hasError}
	autoFocus
	/>
	{hasError && <FormHelpMessage message={ErrorUtils.getLatestErrorMessage(props.account)} />}
	</View>

I am assuming the request used here also works with recovery codes, but if we can apply any changes if needed.

---

Managing focus and input state
We need to introduce a new state to store determine which input should be shown like isUsingRecoveryCode.
We can apply autoFocus to TextInput like it is done in magic code so that when user switches between text input and magic code, the input always auto focuses.
We can either introduce second state variable called recoveryCode and apply appropriate validation when submitting. Or we can use twoFactorAuthCode for also storing recovery code and determine which validation to apply based on isUsingRecoveryCode.
In both cases we should clear these variables when we switch from one input to another

What alternative solutions did you explore? (Optional)

[MonilBhavsar]

Is this issue already assigned to someone ?

Not yet!

[Vishrut19]

Is this issue already assigned to someone ?

Not yet!

okay thanks. So, I can put up my proposal right?

[MonilBhavsar]

@alitoshmatov thanks for the proposal!
I like switching between the inputs, as users won't be required to change their keyboard on mobile to enter numeric 2FA code(for most of the times).

I am assuming the request used here also works with recovery codes, but if we can apply any changes if needed.

Yes

And, recovery code has 8 characters, So I think we can also update the length check.
Overall, I like this compared to a common input to allow both 2FA and recovery codes. @shawnborton curious for your thoughts. May be we can also discuss in #product.

[MonilBhavsar]

Thanks lauren!
@alitoshmatov can you please update the PR with these translations!

[alitoshmatov]

@MonilBhavsar updated PR and requested review from C+

[melvin-bot]

This issue has not been updated in over 15 days. @shawnborton, @MonilBhavsar, @abdulrahuman5196, @alitoshmatov, @laurenreidexpensify 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!

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @alitoshmatov got assigned: 2023-07-21 09:20:27 Z
• when the PR got merged: 2023-09-20 08:38:25 UTC
• days elapsed: 42

On to the next one 🚀

[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.3.73-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Added recovery code option to 2fa #23390

If no regressions arise, payment will be issued on 2023-10-02. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @abdulrahuman5196 requires payment offer (Reviewer)
• @alitoshmatov requires payment offer (Contributor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[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:

• [@abdulrahuman5196] The PR that introduced the bug has been identified. Link to the PR:
• [@abdulrahuman5196] 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:
• [@abdulrahuman5196] 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:
• [@abdulrahuman5196] Determine if we should create a regression test for this bug.
• [@abdulrahuman5196] 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.
• [@laurenreidexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[laurenreidexpensify]

Payment Summary:

• External issue reporter - N/A @MonilBhavsar internal no payment
• Contributor that fixed the issue - @alitoshmatov $1000, payment issued in Upwork
• Contributor+ that helped on the issue and/or PR - @abdulrahuman5196 $1000, payment issued in Upwork

To note there was also a case for a +9 day penalty here, but am doing a discretionary waiver as the issue remained blocked on Expensify for a few weeks after assignment.

[laurenreidexpensify]

@abdulrahuman5196 bump for steps ahead ^^

[alitoshmatov]

@laurenreidexpensify The pr caused a regression here #28092.

[alitoshmatov]

Though I would argue that this was not a regression but a completion to the original PR and caused by incomplete requirements. I mean original PR didn't break anything and delivered expected result. Considering that this was a feature not a bug, detailed requirements would have definitely been helpful. Still, It is just my opinion, and I respect the final decision

[alitoshmatov]

I have another pending payment issues and we can skip on one of those 500$ payments to fix overpayment

[abdulrahuman5196]

The PR that introduced the bug has been identified. Link to the PR:
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:
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:

Not regression. This is a feature request.

Determine if we should create a regression test for this bug.

Yes.

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.

General regression test for the new feature.

1. Have a user whose 2fa is enabled
2. Sign in with this user
3. At two-factor authentication step make sure Use recovery code text link is present
4. Make sure pressing it switches input type to simple text input
5. Enter recovery code
6. Make sure recovery code is being validated correctly and you can sign in with correct recovery code

cc: @laurenreidexpensify

[laurenreidexpensify]

@MonilBhavsar what are your thoughts - #22335 (comment)

[MonilBhavsar]

Since this issue was a feature request, technically we cannot call #28092 as regression considering that workflow did not exist previously. But It was something that we could have caught while testing the PR linked to this issue.

My thought - we can issue full payment here and consider #28092 as a follow up issue/PR since it was one liner change and issue no payment there. If it is possible and everyone agrees? @laurenreidexpensify @abdulrahuman5196 @alitoshmatov

[abdulrahuman5196]

Agree on the same @MonilBhavsar .

Cc: @laurenreidexpensify

[MonilBhavsar]

Thank you everyone!
@laurenreidexpensify we have closed #28092 without payment and we need to issue payment here

[laurenreidexpensify]

Okay great - as per summary here all payments have been issued, so am closing.