search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.48k stars 2.83k forks source link

[$375] [HOLD for payment 2024-08-01] [Wave Collect] [Xero] Add 2FA requirement in Authorization flow #43015

Closed francoisl closed 2 months ago

francoisl commented 4 months ago

Problem

In order to remain compliant with Xero's third-party app requirements, we need to force workspace admins to enable 2FA before they can use the connection.

Additional internal context

Solution

cc @lakchote @zanyrenney

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f129776196b2ac5f
  • Upwork Job ID: 1821257266596661328
  • Last Price Increase: 2024-08-07
melvin-bot[bot] commented 4 months ago

Triggered auto assignment to @sakluger (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

melvin-bot[bot] commented 4 months ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

melvin-bot[bot] commented 4 months ago

Triggered auto assignment to Design team member for new feature review - @dannymcclain (NewFeature)

francoisl commented 4 months ago

I'll add a few mockups when I have edit access to Figma again.

dannymcclain commented 4 months ago

@francoisl let me know if you need me to mock anything up or take care of anything in Figma! 🤝

francoisl commented 4 months ago

Thanks. I got an email from Figma saying that Jon gave me edit access but I still can't seem to edit anything.

Basically I was trying to edit the Initial Setup part of the Xero mockups and add the 2FA flow in between. I was thinking of making two variations:

dannymcclain commented 4 months ago

Super weird. I see that you were upgraded yesterday, but when I went into the file it still said you were requesting access (I gave it to you). I wonder if that file just hadn't been refreshed or something since Jon upgraded you. Anyways, try it again if you want, or I'm happy to do some jammin'.

dannymcclain commented 4 months ago

Added a little flow here in Figma.

I think it's better to tell people "Hey you need to enable 2fa to use xero" so I like including the intermediary modal.

image

francoisl commented 4 months ago

That's great, thanks Danny!

Looks like I was wrong about the sign-in flow and <ValidateLoginPage> doesn't do what I expected, so we'll also need to make some changes to make people enable 2FA when they sign in. I'm thinking we add an extra step after entering the magic code. Here's a rough draft - Figma here:

image

I changed the text description, but we can also make it more generic to accommodate the case when people sign in and they need to enable 2FA because they're on a domain group with that requirement.

dannymcclain commented 4 months ago

Ah I see, yeah that makes sense to me!

muttmuure commented 4 months ago

Looks great to me

dannymcclain commented 4 months ago

Any updates here?

rushatgabhane commented 4 months ago

yo yo yo sorry i lost track of this issue. can you please assign it to me?

rushatgabhane commented 4 months ago

PR raised https://github.com/Expensify/App/pull/44059

rushatgabhane commented 4 months ago

@francoisl everything tests well but we might have some hiccups on how 2FA works.

For the flow: admin signs in and has a policy connected to Xero.

  1. Login
  2. Start 2FA steps
  3. Scan QR code in authenticator
  4. Paste the 6 digit code

Expected: the app becomes usable again. Actual: the API call to enable 2FA completes but the 2FA loader spins infinitely. The app is unusable until we do a re-login.

https://github.com/Expensify/App/assets/29673073/3adaee56-4c8c-4b20-b7a2-6e3cc23f16c5

francoisl commented 4 months ago

Hm weird. I'll look into this today.

francoisl commented 3 months ago

As far as I can tell, it's because the authToken returned by TwoFactorAuth_Validate is not merged into Onyx before the next API call to ReconnectApp.

In short, when 2FA is required to be enabled on sign-in like in our case here, the backend will first send an authToken that has very limited permissions - one of the very few API commands it can execute is TwoFactorAuth_Validate. That API command then returns a "regular" authToken that can execute other API commands.

As an example here, after signing in let's say I get this authToken 0322AAEA4169... - this is a two-factor setup authToken with limited permissions.

image

Once I enter an OTP code to enable 2FA, the client calls TwoFactorAuth_Validate, which returns a new authToken, in this case E6CE5CDF6621...

image

However, immediately after we see that there is an API call to ReconnectApp, but for some reason it's still using the limited authToken 0322AAEA4169...

image

This is why the backend returns an error "Two Factor Authentication Required" and the app doesn't load.

I think the reason we call ReconnectApp before updating the authToken in Onyx is because of that OnyxUpdates.saveUpdateInformation() call here, though I can't think of how to fix the issue. I tried adding WRITE_COMMANDS.TWO_FACTOR_AUTH_VALIDATE to const requestsToIgnoreLastUpdateID but that doesn't fully solve the issue because then we don't call ReconnectApp at all.

rushatgabhane commented 3 months ago

@francoisl thanks for looking into this! i think we'll have to convert TwoFactorAuth_Validate to API_REQUEST_WITH_SIDE_EFFECTS. We can then wait for the api call to complete and reconnect the app with the new token

melvin-bot[bot] commented 3 months ago

This issue has not been updated in over 15 days. @francoisl, @sakluger, @dannymcclain, @rushatgabhane 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!

francoisl commented 3 months ago

Final PR was just merged earlier today.

melvin-bot[bot] commented 3 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

melvin-bot[bot] commented 3 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

c3024 commented 3 months ago

PR was reverted.

@francoisl Could you please assign me this issue so that when the new PR is opened and marked ready puller bear correctly requests my review?

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

If no regressions arise, payment will be issued on 2024-07-25. :confetti_ball:

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

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 2 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

sakluger commented 2 months ago

I have a few payment-related questions:

  1. @francoisl - this issue is labeled as Internal, but @rushatgabhane made one of the PRs (https://github.com/Expensify/App/pull/44059). Shouldn't this be External instead?
  2. What should the price be on this one - $250 or $500?
  3. It looks like there was a regression from this issue's PR. Do we need to halve the price?
c3024 commented 2 months ago

@sakluger

  1. I reviewed the PR 😆
melvin-bot[bot] commented 2 months ago

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

If no regressions arise, payment will be issued on 2024-08-01. :confetti_ball:

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

melvin-bot[bot] commented 2 months ago

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

rushatgabhane commented 2 months ago

@sakluger i think 500 would be great because it was hard to nail down the nitty gritty of this task.

rushatgabhane commented 2 months ago

@c3024 could you please add regression test steps

c3024 commented 2 months ago

Regression test proposal

Test 1:

Pre-requisite:

Have an account A that

  1. does not have 2FA.
  2. is an admin of a control policy P

Steps:

  1. Login with the above specified account A
  2. Click on Workspaces > Click on the control policy P specified in the pre-requisite > Click on Accounting in the left hand panel
  3. Click on Connect Xero
  4. Verify that 2FA modal is shown
  5. Complete the 2FA setup flow
  6. Verify that the Xero integration page opens now

Test 2:

Pre-requisites:

Have an account A that

  1. does not have 2FA
  2. has a Xero accounting configured policy
  3. is an admin of this policy

Steps:

  1. Login with the above specified account A
  2. Verify that 2FA modal is shown with loading indicator behind
  3. Click on Enable Two Factor Authentication button on the modal
  4. Verify that the Avatar and Name in the left hand panel of Settings page has loading indicator
  5. Click on Two Factor Authentication in the center panel and complete the 2FA setup flow
  6. Verify that now the Avatar and Image in the left hand panel of Settings page is fully loaded with details
  7. Click on Inbox
  8. Verify that LHN and Center Pane load fully
melvin-bot[bot] commented 2 months ago

Payment Summary

Upwork Job

BugZero Checklist (@sakluger)

JmillsExpensify commented 2 months ago

Can I get a payment summary on this issue?

sakluger commented 2 months ago

Sorry about that! I missed it because it wasn't automatically moved to Daily.

I chatted with @francoisl via DM to figure out what was going on here. We won't penalize for regressions here because the app was already unusable if you were an admin on a Xero workspace prior to the first PR.

Regarding price, we both thought that $500 was a bit high, but given the trickiness, we'll pay $375 - a bit more than the standard $250.

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

Current assignees @rushatgabhane and @c3024 are eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 2 months ago

Upwork job price has been updated to $375

sakluger commented 2 months ago

@JmillsExpensify the payment summary is ready: https://github.com/Expensify/App/issues/43015#issuecomment-2263664335

JmillsExpensify commented 2 months ago

$375 approved for @rushatgabhane

sakluger commented 2 months ago

I completed the payment to @c3024 via Upwork.