Skip to content

Add better error handling for Onfido#5316

Merged
marcaaron merged 8 commits into
mainfrom
marcaaron-betterLoggingOnfido
Sep 23, 2021
Merged

Add better error handling for Onfido#5316
marcaaron merged 8 commits into
mainfrom
marcaaron-betterLoggingOnfido

Conversation

@marcaaron

@marcaaron marcaaron commented Sep 17, 2021

Copy link
Copy Markdown
Contributor

cc @Jag96 @ctkochan22 I couldn't get Onfido to work on desktop or web no matter how hard I tried to take a picture of my ID via my phone. But tested this with the current web-secure changes and it caught the weird error about referrer.

Details

Onfido was throwing a pretty clear error but we are not logging it at all or handling it to allow the user to attempt the flow again. This will log these errors and also try to move the user back to the Company Step so they can attempt to go through Onfido again.

Fixed Issues (Only related to does not fix)

https://github.com/Expensify/Expensify/issues/177787#

Tests (iOS only)

In order to test this we need to force the Web-Secure API to do something it shouldn't do like set a bad application id or referrer. For this test I just applied this diff in Web-Secure before testing:

diff --git a/lib/Onfido/API.php b/lib/Onfido/API.php
index 957f53ac..223dad75 100644
--- a/lib/Onfido/API.php
+++ b/lib/Onfido/API.php
@@ -195,6 +195,10 @@ class Onfido_API
         $api = self::getOnfidoApi();
         $sdk_token_request = new Onfido_SdkTokenRequest();
         $sdk_token_request->setApplicantId($applicantID);
+
+        // Add incorrect application id
+        $sdk_token_request->setApplicationId('pizza');
+
         $response = $api->generateSdkToken($sdk_token_request);
         if ($response instanceof Onfido_SdkTokenResponse) {
             return $response->getToken() ?? '';
  1. Create a new account with a non-public domain on the freePlan beta
  2. Create a workspace
  3. Tap "Expensify Card"
  4. Press "Get Started"
  5. Select the "Add account manually" option
  6. Follow instructions for creating a Verifying bank account here
  7. When you reach the Onfido step cancel out of the flow by pressing the blue arrow to the left
  8. Verify you are brought back to the company step
  9. Enter the password to move to the next step again
  10. Press start button and allow camera access
  11. Take a picture of anything
  12. The upload should fail and you should be redirected back to the company step + an error growl should appear like so:
    2021-09-20_11-54-17
  13. Put the app into the background so logs are flushed and tail to find the log that says:

Unexpected application_id: com.chat.expensify.chat, please check the application_id used when generating the token

Other Platforms

Test we can upload documents without issue with and without the diff applied.

QA Steps

None

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Sep 17, 2021

@Jag96 Jag96 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks like a good addition to me, and the comments/context is helpful as well.

I couldn't get Onfido to work on desktop or web no matter how hard I tried to take a picture of my ID via my phone. But tested this with the current web-secure changes and it caught the weird error about referrer.

I think reverting the latest secure change locally would be an easy way to test this 👍

@marcaaron

Copy link
Copy Markdown
Contributor Author

Ok updated the tests here with a diff to apply that will make the error we experienced more noticeable. I was able to run through Onfido on web by using a passport instead of driver's license. Not too sure why I was having issues with that last week. This should be ready now.

@marcaaron marcaaron marked this pull request as ready for review September 20, 2021 22:02
@marcaaron marcaaron requested a review from a team as a code owner September 20, 2021 22:02
@MelvinBot MelvinBot requested review from marcochavezf and removed request for a team September 20, 2021 22:02
@marcaaron

Copy link
Copy Markdown
Contributor Author

P.S. I tested this on Android. And it doesn't look like the onError callback is firing. However, we do end up with this when there's a bad application_id:

2021-09-20_12-41-47

Otherwise, I was able to pass Onfido on Android fine after requesting a good token (i.e. without the test diff applied).

@marcochavezf marcochavezf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code looks good to me, I was able to reproduce until step 12 in the simulator though. The Onfido didn't fail after capturing a black screen and allowed me to continue with the next steps (I tried 3 times with diff accounts and ensured the setApplicationId is set in web-secure), not sure if I'm missing something 🤔

If I cancel the Onfido process the app comes back to the Company Step 👍🏽

Screen Shot 2021-09-21 at 13 41 26

Screen Shot 2021-09-21 at 13 42 28

@marcaaron

Copy link
Copy Markdown
Contributor Author

The Onfido didn't fail after capturing a black screen and allowed me to continue with the next steps (I tried 3 times with diff accounts and ensured the setApplicationId is set in web-secure), not sure if I'm missing something

To clarify, is your simulator build using the local Web-Secure? What do you have set for EXPENSIFY_URL_SECURE in your .env file and/or are you using ngrok or not?

@marcochavezf

Copy link
Copy Markdown
Contributor

The Onfido didn't fail after capturing a black screen and allowed me to continue with the next steps (I tried 3 times with diff accounts and ensured the setApplicationId is set in web-secure), not sure if I'm missing something

To clarify, is your simulator build using the local Web-Secure? What do you have set for EXPENSIFY_URL_SECURE in your .env file and/or are you using ngrok or not?

This is my .env:

Screen Shot 2021-09-22 at 10 45 05

and I think I was using local Web-Secure because I added a breakpoint in lib/Onfido/API.php (added the line $sdk_token_request->setApplicationId('marco-pizza'); ) and the debugger stopped in that line. I'm going to try later today with USE_NGROK=false

Comment thread src/components/Onfido/index.native.js Outdated
Comment thread src/pages/EnablePayments/OnfidoStep.js Outdated
Comment thread src/components/Onfido/index.js Outdated
@marcaaron

Copy link
Copy Markdown
Contributor Author

Updated this one.

@marcochavezf I'm not sure why this isn't working for you but thought it might have to do with using an iOS simulator. So, I re-tested with a sim instead of my hardware device and did not see what you're seeing there.

@marcaaron

Copy link
Copy Markdown
Contributor Author

Here's the error I got while testing iOS simulator:

 LOG  [Error: Encountered an error: upload(OnfidoApiError {
    id = NULL,
    type = bad_application_id,
    message = Unexpected application_id: com.chat.expensify.chat, please check the application_id used when generating the token,
    fields = {}
})]
 DEBUG  [hmmm] Onfido error in RequestorStep - {"error":"Encountered an error: upload(OnfidoApiError {\n    id = NULL,\n    type = bad_application_id,\n    message = Unexpected application_id: com.chat.expensify.chat, please check the application_id used when generating the token,\n    fields = {}\n})"}

@Jag96 Jag96 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tests passed for me!

@marcaaron marcaaron merged commit 99bb7d5 into main Sep 23, 2021
@marcaaron marcaaron deleted the marcaaron-betterLoggingOnfido branch September 23, 2021 19:35
@OSBotify

Copy link
Copy Markdown
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

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by @marcaaron in version: 1.1.1-9 🚀

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

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by @timszot in version: 1.1.2-7 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants