-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[HOLD for payment 2023-07-20] [$1000] "Incorrect magic code" error if click login in 2FA input page #21147
Comments
Triggered auto assignment to @alexpensify ( |
Bug0 Triage Checklist (Main S/O)
|
ProposalPlease re-state the problem that we are trying to solve in this issue."Incorrect magic code" error if click login in 2FA input page What is the root cause of that problem?When user clicked App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js Lines 220 to 250 in 342d4fd
What changes do you think we should make in order to solve the problem?Inside const accountID = lodashGet(props.credentials, 'accountID');
if (accountID) {
Session.signInWithValidateCode(accountID, validateCode, props.preferredLocale, twoFactorAuthCode);
} else {
if (props.credentials.validateCode && twoFactorAuthCode.length !== CONST.TFA_CODE_LENGTH) {
return;
}
Session.signIn('', validateCode, twoFactorAuthCode, props.preferredLocale);
} What alternative solutions did you explore? (Optional)N/A |
ProposalPlease re-state the problem that we are trying to solve in this issue."Incorrect magic code" error if click login in 2FA input page What is the root cause of that problem?When users press
then we will clear account and credential data App/src/libs/actions/Session/index.js Lines 569 to 574 in 1096b6e
Unfortunately, the App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js Lines 237 to 245 in 1096b6e
=> We call API signIn again with the old code => the error is shown What changes do you think we should make in order to solve the problem?If we just check In
we should check if the props.account is empty then early return. And update the dependencies if needed
|
Oh thank you, I've updated, it should be if (props.credentials.validateCode && twoFactorAuthCode.length !== CONST.TFA_CODE_LENGTH) {
return;
} |
ProposalPlease re-state the problem that we are trying to solve in this issue.A sign-in request is unexpectedly fired when pressing the Login button in the Footer while in 2fa form. What is the root cause of that problem?On the Sign In page, it will show the 2FA form only if App/src/pages/signin/SignInPage.js Line 166 in 1096b6e
App/src/pages/signin/SignInPage.js Lines 96 to 101 in 1096b6e
One of the requirements to show the form is the When we press Login in Footer, it will clear both ACCOUNT and CREDENTIALS onyx App/src/libs/actions/Session/index.js Lines 569 to 574 in 1096b6e
The problem is when we are clearing both ACCOUNT and CREDENTIALS Onyx, it will notify the update to the subscriber (component) one by one. In our case, SignInPage will first rerender with an empty (null) ACCOUNT. Empty ACCOUNT won't affect the value of
App/src/pages/signin/ValidateCodeForm/BaseValidateCodeForm.js Lines 220 to 272 in 1096b6e
As the ACCOUNT Onyx is cleared, App/src/components/MagicCodeInput.js Lines 134 to 152 in 1096b6e
Finally, SignInPage will rerender with an empty CREDENTIALS which hides the form. What changes do you think we should make in order to solve the problem?What we want to achieve is, as soon as the user press Login in the Footer, we want to immediately hide the validate code form. As validate code form depends on both ACCOUNT and CREDENTIALS, we should show the form only if both exist and hide if one of them is cleared. ACCOUNT and CREDENTIALS onyx will always have data when we are signing in (CREDENTIALS onyx is set from the API response). App/src/libs/actions/Session/index.js Lines 208 to 219 in a4dfac7
And as explained on the root cause, both are cleared when pressing the Login button in the Footer (it's also being cleared if we press Go back, see img below). Currently, Add What alternative solutions did you explore? (Optional)Clears CREDENTIALS first by switching up the position of ACCOUNT and CREDENTIALS. App/src/libs/actions/Session/index.js Lines 569 to 574 in 1096b6e
|
This is on my test list |
I'm catching up from my half day, I'll review soon. |
There is a similar GH here: #19712. I'm going to test in Staging to confirm if this one is not fixed by the GH I mentioned. |
Job added to Upwork: https://www.upwork.com/jobs/~010bba1d812d6c5f0f |
Current assignee @alexpensify is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane ( |
@sobitneupane - Out the gate, we have some proposals here. Can you review if any will resolve the issue from this GH? Thank you! |
@sobitneupane - have you been able to review the proposals yet? Thank you for the update! |
@bernhardoj I kind of like your proposal of handling the issue from |
@sobitneupane You're right. I didn't think of that case. I updated my proposal to check if the ACCOUNT Onyx is empty or not instead. But with this change, I think I would prefer clearing the CREDENTIALS first solution. |
Triggered auto assignment to @slafortune ( |
Alright, removing the other 🐛 team member. I'm back online and see that this is our current state: Prepare for automation! |
Still waiting on automation here |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.39-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-07-20. 🎊 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.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
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:
|
@gadhiyamanan and @bernhardoj - I've prepared your payments in Upwork. Please accept and I can complete the next steps. @sobitneupane please complete the checklist and request payment. Thank you! |
@alexpensify offer accepted, thanks! |
@gadhiyamanan and @bernhardoj - I've completed your payment in Upwork. |
@sobitneupane please complete the checklist and then I can close out this GH. Thank you! |
Thanks! |
@anmurali - we are on hold for C+ payment and will give the 🟢 as soon as the checklist has been completed. |
@sobitneupane when you get a chance, please complete the checklist so we can close this one out. Thank you! |
This is an edge case. I don't think this could have been caught in PR reivew.
yes.
|
Regression Test Proposal
Do we agree 👍 or 👎 |
Payment Requested on newDot. |
@sobitneupane, thank you! @jasperhuangg can you please review the checklist and give a 👍🏼 ? Thanks |
Here is the payment summary:
Upwork Job: *If applicable, the bonuses will be applied on the final payment Extra Notes regarding payment: N/A |
@JmillsExpensify - the payment summary has been added here. It's ready for payment-- thank you! |
Reviewed details for @sobitneupane. This is accurate based on summary from Business Reviewer and approved for payment in NewDot. |
Closing - no update here, so I'm going to close this out. I closed the job in Upwork too. |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
should not show "Incorrect magic code" error
Actual Result:
shows "Incorrect magic code" error
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.29-8
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):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Screen.Recording.2023-06-15.at.11.02.25.AM.mov
Recording.1038.mp4
Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686808469824509
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: