From 585c79a738e0768ce53a4932dc5e5efc51168986 Mon Sep 17 00:00:00 2001 From: Szymon Lesisz Date: Fri, 12 Apr 2019 16:22:57 +0200 Subject: [PATCH 1/7] fix: setInitialUrl - race condition - Do not set initialUrl if it is landingPage - Block "RouterAction.selectDevice" if currently selected device is in auth process - Move WalletActions.observe() before DiscoveryActions.restore() in "WalletSerivce" --- src/actions/RouterActions.js | 55 +++++++++++++++++++------------ src/services/WalletService.js | 61 +++++++++++++++++++---------------- 2 files changed, 67 insertions(+), 49 deletions(-) diff --git a/src/actions/RouterActions.js b/src/actions/RouterActions.js index d078baf1..4b7ad51c 100644 --- a/src/actions/RouterActions.js +++ b/src/actions/RouterActions.js @@ -430,29 +430,42 @@ export const setInitialUrl = (): PayloadAction => ( getState: GetState ): boolean => { const { initialPathname } = getState().wallet; - if (typeof initialPathname === 'string' && !dispatch(isLandingPageUrl(initialPathname, true))) { - const valid = dispatch( - getValidUrl({ - type: LOCATION_CHANGE, - payload: { - location: { - pathname: initialPathname, - hash: '', - search: '', - state: {}, - }, + if (typeof initialPathname !== 'string') return false; + + // DEVICE.CONNECT race condition, "selectDevice" method was called but currently selectedDevice is in getState (auth) process + // if so, consume this action (return true) to break "selectDevice" process + // "setInitialUrl" will be called again after AUTH_DEVICE action + const { selectedDevice } = getState().wallet; + if ( + selectedDevice && + selectedDevice.type === 'acquired' && + !selectedDevice.features.passphrase_protection && + !selectedDevice.state + ) + return true; + + const valid = dispatch( + getValidUrl({ + type: LOCATION_CHANGE, + payload: { + location: { + pathname: initialPathname, + hash: '', + search: '', + state: {}, }, - }) - ); + }, + }) + ); - if (valid === initialPathname) { - // reset initial url - dispatch({ - type: SET_INITIAL_URL, - }); - dispatch(goto(valid)); - return true; - } + if (valid === initialPathname) { + // reset initial url + dispatch({ + type: SET_INITIAL_URL, + }); + dispatch(goto(valid)); + return true; } + return false; }; diff --git a/src/services/WalletService.js b/src/services/WalletService.js index 569e03b6..5d0a75d9 100644 --- a/src/services/WalletService.js +++ b/src/services/WalletService.js @@ -29,11 +29,16 @@ const WalletService: Middleware = (api: MiddlewareAPI) => (next: MiddlewareDispa api.dispatch(WalletActions.init()); // set initial url // TODO: validate if initial url is potentially correct - api.dispatch({ - type: WALLET.SET_INITIAL_URL, - pathname: action.payload.location.pathname, - state: {}, - }); + // exclude landing page url + const { pathname } = action.payload.location; + const isValidPath = !api.dispatch(RouterActions.isLandingPageUrl(pathname, true)); + if (isValidPath) { + api.dispatch({ + type: WALLET.SET_INITIAL_URL, + pathname: action.payload.location.pathname, + state: {}, + }); + } // pass action and break process return next(action); } @@ -63,6 +68,29 @@ const WalletService: Middleware = (api: MiddlewareAPI) => (next: MiddlewareDispa // update common values ONLY if application is ready if (!api.getState().wallet.ready) return action; + // observe common values in WallerReducer + if (!(await api.dispatch(WalletActions.observe(prevState, action)))) { + // if "selectedDevice" didn't change observe common values in SelectedAccountReducer + if (!(await api.dispatch(SelectedAccountActions.observe(prevState, action)))) { + // if "selectedAccount" didn't change observe send form props changes + api.dispatch(SendFormActions.observe(prevState, action)); + } + } else { + // no changes in common values + if (action.type === CONNECT.RECEIVE_WALLET_TYPE) { + if (action.device.state) { + // redirect to root view (Dashboard) if device was authenticated before + api.dispatch(RouterActions.selectFirstAvailableDevice(true)); + } + api.dispatch(TrezorConnectActions.authorizeDevice()); + } + if (action.type === CONNECT.AUTH_DEVICE) { + // selected device did changed + // try to restore discovery after device authentication + api.dispatch(DiscoveryActions.restore()); + } + } + // double verification needed // Corner case: LOCATION_CHANGE was called but pathname didn't changed (redirection from RouterService) const prevLocation = prevState.router.location; @@ -96,29 +124,6 @@ const WalletService: Middleware = (api: MiddlewareAPI) => (next: MiddlewareDispa api.dispatch(NotificationActions.clear(prevLocation.state, currentLocation.state)); } - // observe common values in WallerReducer - if (!(await api.dispatch(WalletActions.observe(prevState, action)))) { - // if "selectedDevice" didn't change observe common values in SelectedAccountReducer - if (!(await api.dispatch(SelectedAccountActions.observe(prevState, action)))) { - // if "selectedAccount" didn't change observe send form props changes - api.dispatch(SendFormActions.observe(prevState, action)); - } - } else { - // no changes in common values - if (action.type === CONNECT.RECEIVE_WALLET_TYPE) { - if (action.device.state) { - // redirect to root view (Dashboard) if device was authenticated before - api.dispatch(RouterActions.selectFirstAvailableDevice(true)); - } - api.dispatch(TrezorConnectActions.authorizeDevice()); - } - if (action.type === CONNECT.AUTH_DEVICE) { - // selected device did changed - // try to restore discovery after device authentication - api.dispatch(DiscoveryActions.restore()); - } - } - // even if "selectedDevice" didn't change because it was updated on DEVICE.CHANGED before DEVICE.CONNECT action // try to restore discovery if (action.type === DEVICE.CONNECT) { From a797a529310ceceefcd8838d9e346dd2d790ea58 Mon Sep 17 00:00:00 2001 From: Szymon Lesisz Date: Fri, 12 Apr 2019 16:48:49 +0200 Subject: [PATCH 2/7] exclude landing page url fix --- src/actions/RouterActions.js | 1 - src/services/WalletService.js | 15 +++++++-------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/actions/RouterActions.js b/src/actions/RouterActions.js index 4b7ad51c..603bbb01 100644 --- a/src/actions/RouterActions.js +++ b/src/actions/RouterActions.js @@ -434,7 +434,6 @@ export const setInitialUrl = (): PayloadAction => ( // DEVICE.CONNECT race condition, "selectDevice" method was called but currently selectedDevice is in getState (auth) process // if so, consume this action (return true) to break "selectDevice" process - // "setInitialUrl" will be called again after AUTH_DEVICE action const { selectedDevice } = getState().wallet; if ( selectedDevice && diff --git a/src/services/WalletService.js b/src/services/WalletService.js index 5d0a75d9..6c57b8d2 100644 --- a/src/services/WalletService.js +++ b/src/services/WalletService.js @@ -32,13 +32,12 @@ const WalletService: Middleware = (api: MiddlewareAPI) => (next: MiddlewareDispa // exclude landing page url const { pathname } = action.payload.location; const isValidPath = !api.dispatch(RouterActions.isLandingPageUrl(pathname, true)); - if (isValidPath) { - api.dispatch({ - type: WALLET.SET_INITIAL_URL, - pathname: action.payload.location.pathname, - state: {}, - }); - } + api.dispatch({ + type: WALLET.SET_INITIAL_URL, + pathname: isValidPath ? pathname : null, + state: {}, + }); + // pass action and break process return next(action); } @@ -48,7 +47,7 @@ const WalletService: Middleware = (api: MiddlewareAPI) => (next: MiddlewareDispa switch (action.type) { case WALLET.SET_INITIAL_URL: - if (action.pathname) { + if (action.hasOwnProperty('pathname')) { api.dispatch(LocalStorageActions.loadData()); } break; From 5b98deac1e13fa252711777a7704dc1b02865252 Mon Sep 17 00:00:00 2001 From: Szymon Lesisz Date: Fri, 12 Apr 2019 16:51:42 +0200 Subject: [PATCH 3/7] flow fix --- src/actions/WalletActions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/actions/WalletActions.js b/src/actions/WalletActions.js index f8d8765f..98037319 100644 --- a/src/actions/WalletActions.js +++ b/src/actions/WalletActions.js @@ -23,7 +23,7 @@ export type WalletAction = | { type: typeof WALLET.SET_INITIAL_URL, state?: RouterLocationState, - pathname?: string, + pathname?: ?string, } | { type: typeof WALLET.TOGGLE_DEVICE_DROPDOWN, From 6cfff6d5986ad5eacdf2e64e4eb5c01c7575a995 Mon Sep 17 00:00:00 2001 From: Szymon Lesisz Date: Fri, 12 Apr 2019 17:13:42 +0200 Subject: [PATCH 4/7] cleaner code for WALLET.SET_INITIAL_URL --- src/actions/WalletActions.js | 2 +- src/services/WalletService.js | 19 +++++++++---------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/actions/WalletActions.js b/src/actions/WalletActions.js index 98037319..f8d8765f 100644 --- a/src/actions/WalletActions.js +++ b/src/actions/WalletActions.js @@ -23,7 +23,7 @@ export type WalletAction = | { type: typeof WALLET.SET_INITIAL_URL, state?: RouterLocationState, - pathname?: ?string, + pathname?: string, } | { type: typeof WALLET.TOGGLE_DEVICE_DROPDOWN, diff --git a/src/services/WalletService.js b/src/services/WalletService.js index 6c57b8d2..e0dc6a60 100644 --- a/src/services/WalletService.js +++ b/src/services/WalletService.js @@ -32,12 +32,13 @@ const WalletService: Middleware = (api: MiddlewareAPI) => (next: MiddlewareDispa // exclude landing page url const { pathname } = action.payload.location; const isValidPath = !api.dispatch(RouterActions.isLandingPageUrl(pathname, true)); - api.dispatch({ - type: WALLET.SET_INITIAL_URL, - pathname: isValidPath ? pathname : null, - state: {}, - }); - + if (isValidPath) { + api.dispatch({ + type: WALLET.SET_INITIAL_URL, + pathname, + state: {}, + }); + } // pass action and break process return next(action); } @@ -46,10 +47,8 @@ const WalletService: Middleware = (api: MiddlewareAPI) => (next: MiddlewareDispa next(action); switch (action.type) { - case WALLET.SET_INITIAL_URL: - if (action.hasOwnProperty('pathname')) { - api.dispatch(LocalStorageActions.loadData()); - } + case WALLET.SET_FIRST_LOCATION_CHANGE: + api.dispatch(LocalStorageActions.loadData()); break; case WALLET.SET_SELECTED_DEVICE: // try to authorize device From 13b49c32b5c6db53eea8647e2b7c8214fd5ed773 Mon Sep 17 00:00:00 2001 From: Szymon Lesisz Date: Fri, 12 Apr 2019 17:27:24 +0200 Subject: [PATCH 5/7] Update RouterActions.js --- src/actions/RouterActions.js | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/actions/RouterActions.js b/src/actions/RouterActions.js index 603bbb01..f8f10877 100644 --- a/src/actions/RouterActions.js +++ b/src/actions/RouterActions.js @@ -429,19 +429,13 @@ export const setInitialUrl = (): PayloadAction => ( dispatch: Dispatch, getState: GetState ): boolean => { - const { initialPathname } = getState().wallet; - if (typeof initialPathname !== 'string') return false; - // DEVICE.CONNECT race condition, "selectDevice" method was called but currently selectedDevice is in getState (auth) process // if so, consume this action (return true) to break "selectDevice" process const { selectedDevice } = getState().wallet; - if ( - selectedDevice && - selectedDevice.type === 'acquired' && - !selectedDevice.features.passphrase_protection && - !selectedDevice.state - ) - return true; + if (selectedDevice && selectedDevice.type === 'acquired' && !selectedDevice.state) return true; + + const { initialPathname } = getState().wallet; + if (typeof initialPathname !== 'string') return false; const valid = dispatch( getValidUrl({ From 89d8485bbd46cc34b73a135db8f9f40849c0af4c Mon Sep 17 00:00:00 2001 From: Szymon Lesisz Date: Mon, 15 Apr 2019 11:34:49 +0200 Subject: [PATCH 6/7] remove unnecesary condition from WalletAction.observe --- src/actions/WalletActions.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/actions/WalletActions.js b/src/actions/WalletActions.js index f8d8765f..1aa9a853 100644 --- a/src/actions/WalletActions.js +++ b/src/actions/WalletActions.js @@ -169,15 +169,11 @@ export const observe = (prevState: State, action: Action): PayloadAction Date: Mon, 15 Apr 2019 11:35:14 +0200 Subject: [PATCH 7/7] Update WalletService.js --- src/services/WalletService.js | 46 ++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/src/services/WalletService.js b/src/services/WalletService.js index e0dc6a60..0578c84e 100644 --- a/src/services/WalletService.js +++ b/src/services/WalletService.js @@ -66,28 +66,8 @@ const WalletService: Middleware = (api: MiddlewareAPI) => (next: MiddlewareDispa // update common values ONLY if application is ready if (!api.getState().wallet.ready) return action; - // observe common values in WallerReducer - if (!(await api.dispatch(WalletActions.observe(prevState, action)))) { - // if "selectedDevice" didn't change observe common values in SelectedAccountReducer - if (!(await api.dispatch(SelectedAccountActions.observe(prevState, action)))) { - // if "selectedAccount" didn't change observe send form props changes - api.dispatch(SendFormActions.observe(prevState, action)); - } - } else { - // no changes in common values - if (action.type === CONNECT.RECEIVE_WALLET_TYPE) { - if (action.device.state) { - // redirect to root view (Dashboard) if device was authenticated before - api.dispatch(RouterActions.selectFirstAvailableDevice(true)); - } - api.dispatch(TrezorConnectActions.authorizeDevice()); - } - if (action.type === CONNECT.AUTH_DEVICE) { - // selected device did changed - // try to restore discovery after device authentication - api.dispatch(DiscoveryActions.restore()); - } - } + // check for "selectedDevice" change before any action + const selectedDeviceDidChange = await api.dispatch(WalletActions.observe(prevState, action)); // double verification needed // Corner case: LOCATION_CHANGE was called but pathname didn't changed (redirection from RouterService) @@ -122,6 +102,28 @@ const WalletService: Middleware = (api: MiddlewareAPI) => (next: MiddlewareDispa api.dispatch(NotificationActions.clear(prevLocation.state, currentLocation.state)); } + // if "selectedDevice" didn't change observe common values in SelectedAccountReducer + if (!selectedDeviceDidChange) { + if (!(await api.dispatch(SelectedAccountActions.observe(prevState, action)))) { + // if "selectedAccount" didn't change observe send form props changes + api.dispatch(SendFormActions.observe(prevState, action)); + } + } else { + // no changes in common values + if (action.type === CONNECT.RECEIVE_WALLET_TYPE) { + if (action.device.state) { + // redirect to root view (Dashboard) if device was authenticated before + api.dispatch(RouterActions.selectFirstAvailableDevice(true)); + } + api.dispatch(TrezorConnectActions.authorizeDevice()); + } + if (action.type === CONNECT.AUTH_DEVICE) { + // selected device did changed + // try to restore discovery after device authentication + api.dispatch(DiscoveryActions.restore()); + } + } + // even if "selectedDevice" didn't change because it was updated on DEVICE.CHANGED before DEVICE.CONNECT action // try to restore discovery if (action.type === DEVICE.CONNECT) {