feat(INJI-205): Error handling in OIDC flow #878

vharsh · 2023-09-29T07:36:04Z

User visible scenarios handled

[DONE] On loss of internet connectivity --> No internet connectivity message shown
[DONE] Invalid broken URL --> the library will pass a config error hence this will result in a generic error
[DONE] Request timeout --> Something went wrong message shown
[DONE] User cancels OpenID4VC flow --> back button is pressed, no error shown as it's not an error and the error is handled for both Android & iOS platforms.

Code changes

issuerMachine: add intermediary states for managing errors and internet connectivity checks
add an optional param in shared/request.ts for requestTimeout without breaking other's changes

What needs to be improved ?

~~efficacy of timeout handling on fetch is sort of debatable, it'd need to be tested well~~ this has been implemented using AbortController's polyfill added in ReactNative 0.60, hence it works on React Native 0.60 & above

Request timeout feature implementation

AbortController was added from RN 0.60 using an AbortController polyfill(this worked on iPhone 7 & Nokia 6.1+) using this sample code

...
const banana = request('GET', '/', undefined, 2, 'http://[REDACTED]')
          .then((x) => console.log('Success:', x))
          .catch((e) => console.log('Banana Failure', e));
...

The server writing the response for the above request had a sleep for 10seconds, the server did wait for 10seconds to write the response but from the client logs the Promise got rejected with this log. The logs are sort of a real-world proof that the timeout should work on physical devices.

// iPhone 7 test
 LOG  [Harsh Vardhan’s iPhone] app: ONLINE -> ready.focus.checking
{
  "networkType": "wifi"
}
 LOG  Banana Failure [AbortError: Aborted]

// Android 10, Nokia 6.1+ test

 LOG  [46a5f0c3] app: ONLINE -> ready.focus.checking
{
  "networkType": "wifi"
}
 LOG  Banana Failure [AbortError: Aborted]

swatigoel · 2023-10-04T07:37:45Z

shared/request.ts

@@ -16,30 +16,41 @@ export async function request(
  method: 'GET' | 'POST' | 'PATCH',
  path: `/${string}`,
  body?: Record<string, unknown>,
+  timeout?: undefined | number,


Let's add timeout as last parameter as it's optional.

After this change, I am hoping QR login will not work as we are sending ESIGNET_HOST as last argument in function call. Please check it once.

I'm a bit confused with the definitive/recommended order of parameters in typescript and I've not found the guidelines on the order of parameters of different types such as normal, optional and default.

machines/issuersMachine.ts

vijay151096 · 2023-10-10T06:44:08Z

machines/issuersMachine.ts

+            {
+              description: 'not fetched issuers config yet',
+              cond: 'shouldFetchIssuersAgain',
+              actions: ['setLoadingIssuer', 'resetError'],


since we are updating the loadingReason context, can we rename setLoadingIssuer to setLoadingReason.

we are updating loading reason to be 'loading issuers' and in other areas we update loading reason to downloading credentials & so. so we are using setLoadingIssuers name for the action to differentiate loadingReason setup for different areas.

Renamed function accordingly

machines/issuersMachine.ts

vijay151096 · 2023-10-10T06:54:56Z

machines/issuersMachine.ts

+      setDownloadingCreds: model.assign({
+        loadingReason: 'downloadingCredentials',
+      }),
+      setTokenLoadingReason: model.assign({


instead of separate setter for Loading Reason context variable, can't we pass a argument and unify them

You mean like inline actions? if so, Wouldn't inline actions make it hard to debug / visualize the machine?

Renamed function accordingly

machines/issuersMachine.ts

shared/GlobalContext.ts

- `internet not available` on network request failure to issuer - generic error message on other errors in the flow - OIDC flow cancel error message Signed-off-by: Kiruthika Jeyashankar <[email protected]>

Signed-off-by: Kiruthika Jeyashankar <[email protected]>

granular level changes: - merged error states - move to a two-step issuer-ID assign of issuer from issuerScreen to state machine - improved OIDC cancel flow handling for android/iOS openid platform libs Co-authored-by: Kiruthika Jeyashankar <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

Co-authored-by: Kiruthika Jeyashankar <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

this feature was added in [RN 0.60](https://github.com/react-native-community/releases/blob/master/CHANGELOG.md#v0600) Signed-off-by: Kiruthika Jeyashankar <[email protected]>

Signed-off-by: Kiruthika Jeyashankar <[email protected]>

Co-authored-by: Kiruthika Jeyashankar <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

the timeout is kept as 30 seconds Co-authored-by: Harsh Vardhan <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

Co-authored-by: Harsh Vardhan <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

Co-authored-by: Kiruthika Jeyashankar <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

Signed-off-by: Kiruthika Jeyashankar <[email protected]>

vijay151096 · 2023-10-10T10:25:15Z

shared/openId4VCI/Utils.ts

@@ -92,3 +92,20 @@ export const getJWT = async context => {
    throw e;
  }
 };
+export const VC_DOWNLOAD_TIMEOUT = 30;


can we move this property to inji-default.properties

yes, let's move it to inji-default.properties and add as Initial config which is being used for caching

Change included

vijay151096 · 2023-10-10T10:26:17Z

shared/openId4VCI/Utils.ts

+  REQUEST_TIMEDOUT = 'requestTimedOut',
+}
+export const NETWORK_REQUEST_FAILED = 'Network request failed';
+export const REQUEST_TIMEOUT = 'request timedout';


request.ts is not using this constant REQUEST_TIMEOUT variable

Co-authored-by: Harsh Vardhan <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

vijay151096 · 2023-10-10T13:21:06Z

machines/issuersMachine.ts

-              actions: ['setPublicKey', 'setPrivateKey', 'storeKeyPair'],
+              actions: [
+                'setPublicKey',
+                'setLoadingReasonAsDownloadingCreds',


please remove the abbreviation "creds" and use full name for more clarity "credential"

vijay151096 · 2023-10-10T13:22:09Z

machines/issuersMachine.ts

-  return state.context.errorMessage;
+export function selectErrorMessageType(state: State) {
+  return state.context.errorMessage === '' ||
+    state.context.errorMessage === 'noInternetConnection'


please use the ErrorMessage enums here as well

vijay151096 · 2023-10-10T13:22:36Z

screens/Issuers/IssuersScreen.tsx

@@ -50,14 +54,18 @@ export const IssuersScreen: React.FC<
  };

  const isGenericError = () => {
-    return controller.errorMessage === 'generic';
+    return controller.errorMessageType === 'generic';


use ErrorMessage enum here as well.

vijay151096 · 2023-10-10T13:24:54Z

shared/openId4VCI/Utils.ts

@@ -92,3 +93,19 @@ export const getJWT = async context => {
    throw e;
  }
 };
+export const VC_DOWNLOAD_TIMEOUT =
+  Number(getConfig('openId4VCIDownloadVCTimeout')) || 30000;


dont need default value 30000 here, as cached response itself will set it, if not avl.

If the promise fails to resolve or the key is missing from the server, the promise.catch would return '' and Number('') would evaluate to 0, hence this or condition is required.

vijay151096 · 2023-10-10T13:25:32Z

shared/commonprops/commonProps.ts

@@ -10,6 +10,14 @@ export default async function getAllConfigurations(host = undefined) {
  return await CACHED_API.getAllProperties();
 }

+export function getConfig(key: string): string {


please remove this function and use getAllConfiguration, no need for separate function.

This is a helper function we've written for future convenience usage

Refactored to use getAllConfigurations

Signed-off-by: Kiruthika Jeyashankar <[email protected]>

Co-authored-by: Harsh Vardhan <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

vijay151096

changes looks good to me.

vharsh marked this pull request as ready for review October 4, 2023 05:40

swatigoel reviewed Oct 4, 2023

View reviewed changes

vharsh force-pushed the feature-205-error-vc-download branch 5 times, most recently from 962512b to 9f8c778 Compare October 9, 2023 06:28

KiruthikaJeyashankar force-pushed the feature-205-error-vc-download branch from 07eea13 to 4347a96 Compare October 9, 2023 12:57

vijay151096 reviewed Oct 10, 2023

View reviewed changes

machines/issuersMachine.ts Outdated Show resolved Hide resolved

vijay151096 reviewed Oct 10, 2023

View reviewed changes

machines/issuersMachine.ts Outdated Show resolved Hide resolved

vijay151096 reviewed Oct 10, 2023

View reviewed changes

machines/issuersMachine.ts Outdated Show resolved Hide resolved

vijay151096 reviewed Oct 10, 2023

View reviewed changes

machines/issuersMachine.ts Outdated Show resolved Hide resolved

vijay151096 reviewed Oct 10, 2023

View reviewed changes

shared/GlobalContext.ts Show resolved Hide resolved

vharsh and others added 12 commits October 10, 2023 13:34

feat(INJI-205): handle errors in OIDC flow in issuerMachine

b57af95

- `internet not available` on network request failure to issuer - generic error message on other errors in the flow - OIDC flow cancel error message Signed-off-by: Kiruthika Jeyashankar <[email protected]>

feat(INJI-205): add support for request timeout for OIDC

256c036

Signed-off-by: Kiruthika Jeyashankar <[email protected]>

refactor(INJI-205): Check Internet before OIDC AuthZ

84715a3

Co-authored-by: Kiruthika Jeyashankar <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

refactor(INJI-205): remove redundant error state

39e51a5

Co-authored-by: Kiruthika Jeyashankar <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

feat(INJI-205): use AbortController's signal timeout

017009f

this feature was added in [RN 0.60](https://github.com/react-native-community/releases/blob/master/CHANGELOG.md#v0600) Signed-off-by: Kiruthika Jeyashankar <[email protected]>

refactor(INJI-205): rename variable and remove redundant comments

fcf4204

Signed-off-by: Kiruthika Jeyashankar <[email protected]>

feat(INJI-205): handle invalid OIDC configs error

de7dfed

Co-authored-by: Kiruthika Jeyashankar <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

feat(INJI-205): handle timeout error for vc download via issuer

8e16ca0

the timeout is kept as 30 seconds Co-authored-by: Harsh Vardhan <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

refactor(INJI-205): remove redundant storing of error message and type

198ac72

Co-authored-by: Harsh Vardhan <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

refactor(INJI-205): move loadingReason update to diff action

b0c2f2c

Co-authored-by: Kiruthika Jeyashankar <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

refactor(INJI-205): rename state actions & extract consts

bcd500f

Co-authored-by: Kiruthika Jeyashankar <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

KiruthikaJeyashankar force-pushed the feature-205-error-vc-download branch from afc30a5 to bcd500f Compare October 10, 2023 08:08

fix(INJI-205): reference errormessages from util in issuersMachine

d9077ff

Signed-off-by: Kiruthika Jeyashankar <[email protected]>

vijay151096 reviewed Oct 10, 2023

View reviewed changes

KiruthikaJeyashankar and others added 5 commits October 10, 2023 16:19

refactor(INJI-205): move common error const to consts file

6192c22

Co-authored-by: Harsh Vardhan <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

refactor(INJI-205): rename action names

a81f27a

Co-authored-by: Harsh Vardhan <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

feat(INJI-205): move to prev screen on back btn press in error screen

eb7396e

Co-authored-by: Harsh Vardhan <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

feat(INJI-205): use openId4VCIDownloadVCTimeout from config

41755bc

Co-authored-by: Harsh Vardhan <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

feat(INJI-205): toggle on openID for VC

9d3c9c2

Co-authored-by: Harsh Vardhan <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

vijay151096 reviewed Oct 10, 2023

View reviewed changes

KiruthikaJeyashankar and others added 2 commits October 10, 2023 18:59

refactor(INJI-205): use extracted error constant to avoid hardcoding

25c2bd6

Signed-off-by: Kiruthika Jeyashankar <[email protected]>

fix(INJI-205): vcDownloadTimeout config fetch

2e91aef

Co-authored-by: Harsh Vardhan <[email protected]> Signed-off-by: Kiruthika Jeyashankar <[email protected]>

vijay151096 approved these changes Oct 10, 2023

View reviewed changes

vijay151096 merged commit e5963cf into mosip:develop Oct 10, 2023
1 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(INJI-205): Error handling in OIDC flow #878

feat(INJI-205): Error handling in OIDC flow #878

vharsh commented Sep 29, 2023 •

edited

Loading

swatigoel Oct 4, 2023

swatigoel Oct 4, 2023

vharsh Oct 4, 2023

vijay151096 Oct 10, 2023

KiruthikaJeyashankar Oct 10, 2023

KiruthikaJeyashankar Oct 10, 2023

vijay151096 Oct 10, 2023

KiruthikaJeyashankar Oct 10, 2023 •

edited

Loading

KiruthikaJeyashankar Oct 10, 2023

vijay151096 Oct 10, 2023

swatigoel Oct 10, 2023

KiruthikaJeyashankar Oct 10, 2023

vijay151096 Oct 10, 2023

KiruthikaJeyashankar Oct 10, 2023

vijay151096 Oct 10, 2023

KiruthikaJeyashankar Oct 10, 2023

vijay151096 Oct 10, 2023

KiruthikaJeyashankar Oct 10, 2023

vijay151096 Oct 10, 2023

KiruthikaJeyashankar Oct 10, 2023

vijay151096 Oct 10, 2023

vharsh Oct 10, 2023

vijay151096 Oct 10, 2023

vharsh Oct 10, 2023

KiruthikaJeyashankar Oct 10, 2023

vijay151096 left a comment

feat(INJI-205): Error handling in OIDC flow #878

feat(INJI-205): Error handling in OIDC flow #878

Conversation

vharsh commented Sep 29, 2023 • edited Loading

User visible scenarios handled

Code changes

What needs to be improved ?

Request timeout feature implementation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KiruthikaJeyashankar Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vijay151096 left a comment

Choose a reason for hiding this comment

vharsh commented Sep 29, 2023 •

edited

Loading

KiruthikaJeyashankar Oct 10, 2023 •

edited

Loading