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.

Sign up for GitHub

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

Jump to bottom

Deprecate requiresTwoFactorAuth property #7864

Merged
merged 1 commit into from
Feb 25, 2022
Merged

Deprecate requiresTwoFactorAuth property #7864

merged 1 commit into from
Feb 25, 2022

Conversation

MariaHCD
Copy link
Contributor

@MariaHCD MariaHCD commented Feb 22, 2022 •
edited
Loading

Details

Remove usage of the requiresTwoFactorAuth property from the response of GetAccountStatus request. This will be removed from the API. Related Web PR: https://github.com/Expensify/Web-Expensify/pull/33141

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/193728

Tests

Tested with https://github.com/Expensify/Web-Expensify/pull/33141
Account set up with 2FA

  1. Login with email and password
  2. After entering the password and clicking signin, verify the 2FA input box is displayed
  3. Enter 2FA code and verify that the user can log in

Account not set up with 2FA

  1. Login in with email and password
  2. Verify the 2FA input box is not displayed
  3. Verify the user can log in normally

QA Steps

Account set up with 2FA

  1. Login with email and password
  2. After entering the password and clicking signin, verify the 2FA input box is displayed. Verify that leaving either or both password or 2FA empty results in a validation error.
  3. Enter 2FA code and verify that the user can log in

Account not set up with 2FA

  1. Login in with email and password
  2. Verify the 2FA input box is not displayed
  3. Verify the user can log in normally

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2022-02-22 at 6 24 24 PM

Desktop

Screen Shot 2022-02-22 at 6 34 12 PM

Screen Shot 2022-02-22 at 6 44 15 PM

Screen Shot 2022-02-22 at 6 44 28 PM

@MariaHCD MariaHCD self-assigned this Feb 22, 2022
@MariaHCD MariaHCD marked this pull request as ready for review February 22, 2022 14:47
@MariaHCD MariaHCD requested a review from a team as a code owner February 22, 2022 14:47
@MelvinBot MelvinBot requested review from sketchydroide and removed request for a team February 22, 2022 14:47
@MariaHCD MariaHCD requested review from stitesExpensify, iwiznia and a team and removed request for sketchydroide February 22, 2022 14:48
@MelvinBot MelvinBot requested review from sketchydroide and removed request for a team February 22, 2022 14:48
stitesExpensify
stitesExpensify approved these changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@stitesExpensify stitesExpensify left a comment

Choose a reason for hiding this comment

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

Can we auto-focus the 2fa input? It feels tedious to have to click after you thought you were already signed in. If you don't want to do it yourself, it would probably be an easy contributor task too

iwiznia
iwiznia requested changes Feb 22, 2022
View reviewed changes
src/libs/actions/Session/index.js
@@ -250,6 +249,10 @@ function signIn(password, twoFactorAuthCode) {
createTemporaryLogin(authToken, email);
})
.catch((error) => {
if (error.message === 'passwordForm.error.twoFactorAuthenticationEnabled') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get this. Where is error.message coming from? How could it equal `'passwordForm.error.twoFactorAuthenticationEnabled'??
Also, we should not base logic on error messages, we should use the jsonCodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is where we're throw the error if we receive a 402 json code when authenticating. Maybe it would be better to return the response in API.js here if the json code is 402 and then change the logic here to:

Suggested change
if (error.message === 'passwordForm.error.twoFactorAuthenticationEnabled') {
if (response.jsonCode === 402) {

Copy link
Contributor

@sketchydroide sketchydroide Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think we use both as in here
We should check for the json code first, and then if the specific mesage is important then check against it as well, the message should be a constant though, I think

Copy link
Contributor

Choose a reason for hiding this comment

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

@sketchydroide that link is broken, can you re-share it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is the correct link?

App/src/libs/actions/ReimbursementAccount/setupWithdrawalAccount.js

Line 89 in 6d0baa3

if (response.jsonCode === 402) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see. Kind of odd we do that... I guess since it is repeating the pattern this is ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

that was some weird fuckery on the link, I've fixed it

@iwiznia iwiznia merged commit e1de762 into main Feb 25, 2022
@iwiznia iwiznia deleted the maria-deprecate-requiresTwoFactorAuth branch February 25, 2022 22:34
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify OSBotify mentioned this pull request Mar 1, 2022
Closed
68 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Mar 1, 2022

🚀 Deployed to staging by @iwiznia in version: 1.1.41-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 failure ❌

@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2022

🚀 Deployed to staging by @iwiznia in version: 1.1.41-0 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 failure ❌

@NikkiWines NikkiWines mentioned this pull request Mar 2, 2022
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.1.41-6 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@Stutikuls Stutikuls mentioned this pull request Mar 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sketchydroide sketchydroide sketchydroide approved these changes

@iwiznia iwiznia iwiznia approved these changes

@stitesExpensify stitesExpensify stitesExpensify approved these changes

Assignees

@MariaHCD MariaHCD

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MariaHCD @OSBotify @sketchydroide @iwiznia @stitesExpensify