From f70096b8faa09083ecddabf8fa8acb132be69486 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 26 Apr 2018 17:07:58 +0100 Subject: [PATCH 1/9] Fix error handling on session restore Fix a number of failures that meant the excellent error handling we had for failing to restore a session didn't work. 1. .catch on the promise rather than try/catch: it's async 2. Explicit cancel method in SessionRestoreErrorDialog that invokes onFinished with `false` because even with the catch fixed, this was getting the event as its first arg which is truthy, so clicking cancel still deleted your data. 3. DialogButtons: Don't pass onCancel straight into the button event handler as this leaks the MouseEvent through as an argument. Nothing is using it and it exacerbates failures like this because there are surprise arguments. Fixes https://github.com/vector-im/riot-web/issues/6616 --- src/Lifecycle.js | 20 +++++++++---------- .../dialogs/SessionRestoreErrorDialog.js | 10 +++++++--- .../views/elements/DialogButtons.js | 6 +++++- 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/Lifecycle.js b/src/Lifecycle.js index ec1fca2bc6..793b0f956b 100644 --- a/src/Lifecycle.js +++ b/src/Lifecycle.js @@ -215,18 +215,16 @@ function _restoreFromLocalStorage() { if (accessToken && userId && hsUrl) { console.log(`Restoring session for ${userId}`); - try { - return _doSetLoggedIn({ - userId: userId, - deviceId: deviceId, - accessToken: accessToken, - homeserverUrl: hsUrl, - identityServerUrl: isUrl, - guest: isGuest, - }, false).then(() => true); - } catch (e) { + return _doSetLoggedIn({ + userId: userId, + deviceId: deviceId, + accessToken: accessToken, + homeserverUrl: hsUrl, + identityServerUrl: isUrl, + guest: isGuest, + }, false).catch((e) => { return _handleRestoreFailure(e); - } + }).then(() => true); } else { console.log("No previous session found."); return Promise.resolve(false); diff --git a/src/components/views/dialogs/SessionRestoreErrorDialog.js b/src/components/views/dialogs/SessionRestoreErrorDialog.js index 451785197e..401550043b 100644 --- a/src/components/views/dialogs/SessionRestoreErrorDialog.js +++ b/src/components/views/dialogs/SessionRestoreErrorDialog.js @@ -41,10 +41,14 @@ export default React.createClass({ Modal.createTrackedDialog('Session Restore Error', 'Send Bug Report Dialog', BugReportDialog, {}); }, - _continueClicked: function() { + _onContinueClick: function() { this.props.onFinished(true); }, + _onCancelClick: function() { + this.props.onFinished(false); + }, + render: function() { const BaseDialog = sdk.getComponent('views.dialogs.BaseDialog'); const DialogButtons = sdk.getComponent('views.elements.DialogButtons'); @@ -81,8 +85,8 @@ export default React.createClass({ { bugreport } + onPrimaryButtonClick={this._onContinueClick} focus={shouldFocusContinueButton} + onCancel={this._onCancelClick} /> ); }, diff --git a/src/components/views/elements/DialogButtons.js b/src/components/views/elements/DialogButtons.js index c159324c1d..537f906a74 100644 --- a/src/components/views/elements/DialogButtons.js +++ b/src/components/views/elements/DialogButtons.js @@ -39,6 +39,10 @@ module.exports = React.createClass({ focus: PropTypes.bool, }, + _onCancelClick: function() { + this.props.onCancel(); + }, + render: function() { let primaryButtonClassName = "mx_Dialog_primary"; if (this.props.primaryButtonClass) { @@ -53,7 +57,7 @@ module.exports = React.createClass({ { this.props.primaryButton } { this.props.children } - From db1401f48444b08780b1709d67c43f03403e72e3 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 11:19:14 +0100 Subject: [PATCH 2/9] Pass false to onFinished from BaseDialog Everywhere else, onFinished takes a boolean indicating whether the dialog was confirmed on cancelled, and had function that were expecting this variable and getting undefined. --- src/components/views/dialogs/BaseDialog.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/components/views/dialogs/BaseDialog.js b/src/components/views/dialogs/BaseDialog.js index 21a2477c37..7959e7cb10 100644 --- a/src/components/views/dialogs/BaseDialog.js +++ b/src/components/views/dialogs/BaseDialog.js @@ -36,6 +36,9 @@ export default React.createClass({ propTypes: { // onFinished callback to call when Escape is pressed + // Take a boolean which is true is the dialog was dismissed + // with a positive / confirm action or false if it was + // cancelled (from BaseDialog, this is always false). onFinished: PropTypes.func.isRequired, // called when a key is pressed @@ -77,12 +80,12 @@ export default React.createClass({ if (e.keyCode === KeyCode.ESCAPE) { e.stopPropagation(); e.preventDefault(); - this.props.onFinished(); + this.props.onFinished(false); } }, _onCancelClick: function(e) { - this.props.onFinished(); + this.props.onFinished(false); }, render: function() { From 0323f8ed0c0db59746b98443d6d07250beab6cef Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 11:25:13 +0100 Subject: [PATCH 3/9] Wrap exception handling around all of loadSession The user might (probably does) have a session even if we haven't actually tried to load it yet, so wrap the whole loadSession code in the error handler we were using for restoring sessions so we gracefully handle exceptions that happen before trying to restore sessions too. Remove the catch in MatrixChat that sent you to the login screen. This is never the right way to handle an error condition: we should only display the login screen if we successfully determined that the user has no session, or they explicitly chose to blow their sessions away. --- src/Lifecycle.js | 120 +++++++++--------- src/components/structures/MatrixChat.js | 8 +- .../dialogs/SessionRestoreErrorDialog.js | 1 + 3 files changed, 67 insertions(+), 62 deletions(-) diff --git a/src/Lifecycle.js b/src/Lifecycle.js index 793b0f956b..6c35c6ed06 100644 --- a/src/Lifecycle.js +++ b/src/Lifecycle.js @@ -1,6 +1,7 @@ /* Copyright 2015, 2016 OpenMarket Ltd Copyright 2017 Vector Creations Ltd +Copyright 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -65,32 +66,33 @@ import sdk from './index'; * failed. */ export function loadSession(opts) { - const fragmentQueryParams = opts.fragmentQueryParams || {}; - let enableGuest = opts.enableGuest || false; - const guestHsUrl = opts.guestHsUrl; - const guestIsUrl = opts.guestIsUrl; - const defaultDeviceDisplayName = opts.defaultDeviceDisplayName; + return new Promise.resolve().then(() => { + const fragmentQueryParams = opts.fragmentQueryParams || {}; + let enableGuest = opts.enableGuest || false; + const guestHsUrl = opts.guestHsUrl; + const guestIsUrl = opts.guestIsUrl; + const defaultDeviceDisplayName = opts.defaultDeviceDisplayName; - if (!guestHsUrl) { - console.warn("Cannot enable guest access: can't determine HS URL to use"); - enableGuest = false; - } + if (!guestHsUrl) { + console.warn("Cannot enable guest access: can't determine HS URL to use"); + enableGuest = false; + } - if (enableGuest && - fragmentQueryParams.guest_user_id && - fragmentQueryParams.guest_access_token - ) { - console.log("Using guest access credentials"); - return _doSetLoggedIn({ - userId: fragmentQueryParams.guest_user_id, - accessToken: fragmentQueryParams.guest_access_token, - homeserverUrl: guestHsUrl, - identityServerUrl: guestIsUrl, - guest: true, - }, true).then(() => true); - } - - return _restoreFromLocalStorage().then((success) => { + if (enableGuest && + fragmentQueryParams.guest_user_id && + fragmentQueryParams.guest_access_token + ) { + console.log("Using guest access credentials"); + return _doSetLoggedIn({ + userId: fragmentQueryParams.guest_user_id, + accessToken: fragmentQueryParams.guest_access_token, + homeserverUrl: guestHsUrl, + identityServerUrl: guestIsUrl, + guest: true, + }, true).then(() => true); + } + return _restoreFromLocalStorage(); + }).then((success) => { if (success) { return true; } @@ -101,6 +103,8 @@ export function loadSession(opts) { // fall back to login screen return false; + }).catch((e) => { + return _handleLoadSessionFailure(e); }); } @@ -196,43 +200,43 @@ function _registerAsGuest(hsUrl, isUrl, defaultDeviceDisplayName) { // SessionStore to avoid bugs where the view becomes out-of-sync with // localStorage (e.g. teamToken, isGuest etc.) function _restoreFromLocalStorage() { - if (!localStorage) { - return Promise.resolve(false); - } - const hsUrl = localStorage.getItem("mx_hs_url"); - const isUrl = localStorage.getItem("mx_is_url") || 'https://matrix.org'; - const accessToken = localStorage.getItem("mx_access_token"); - const userId = localStorage.getItem("mx_user_id"); - const deviceId = localStorage.getItem("mx_device_id"); + return Promise.resolve().then(() => { + if (!localStorage) { + return Promise.resolve(false); + } + const hsUrl = localStorage.getItem("mx_hs_url"); + const isUrl = localStorage.getItem("mx_is_url") || 'https://matrix.org'; + const accessToken = localStorage.getItem("mx_access_token"); + const userId = localStorage.getItem("mx_user_id"); + const deviceId = localStorage.getItem("mx_device_id"); - let isGuest; - if (localStorage.getItem("mx_is_guest") !== null) { - isGuest = localStorage.getItem("mx_is_guest") === "true"; - } else { - // legacy key name - isGuest = localStorage.getItem("matrix-is-guest") === "true"; - } + let isGuest; + if (localStorage.getItem("mx_is_guest") !== null) { + isGuest = localStorage.getItem("mx_is_guest") === "true"; + } else { + // legacy key name + isGuest = localStorage.getItem("matrix-is-guest") === "true"; + } - if (accessToken && userId && hsUrl) { - console.log(`Restoring session for ${userId}`); - return _doSetLoggedIn({ - userId: userId, - deviceId: deviceId, - accessToken: accessToken, - homeserverUrl: hsUrl, - identityServerUrl: isUrl, - guest: isGuest, - }, false).catch((e) => { - return _handleRestoreFailure(e); - }).then(() => true); - } else { - console.log("No previous session found."); - return Promise.resolve(false); - } + if (accessToken && userId && hsUrl) { + console.log(`Restoring session for ${userId}`); + return _doSetLoggedIn({ + userId: userId, + deviceId: deviceId, + accessToken: accessToken, + homeserverUrl: hsUrl, + identityServerUrl: isUrl, + guest: isGuest, + }, false).then(() => true); + } else { + console.log("No previous session found."); + return Promise.resolve(false); + } + }); } -function _handleRestoreFailure(e) { - console.log("Unable to restore session", e); +function _handleLoadSessionFailure(e) { + console.log("Unable to load session", e); const def = Promise.defer(); const SessionRestoreErrorDialog = @@ -253,7 +257,7 @@ function _handleRestoreFailure(e) { } // try, try again - return _restoreFromLocalStorage(); + return loadSession(); }); } diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index 1eb96c9f11..cdeb99ef53 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -1,7 +1,7 @@ /* Copyright 2015, 2016 OpenMarket Ltd Copyright 2017 Vector Creations Ltd -Copyright 2017 New Vector Ltd +Copyright 2017, 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -351,15 +351,15 @@ export default React.createClass({ guestIsUrl: this.getCurrentIsUrl(), defaultDeviceDisplayName: this.props.defaultDeviceDisplayName, }); - }).catch((e) => { - console.error('Error attempting to load session', e); - return false; }).then((loadedSession) => { if (!loadedSession) { // fall back to showing the login screen dis.dispatch({action: "start_login"}); } }); + // Note we don't catch errors from this: we catch everything within + // loadSession as there's logic there to ask the user if they want + // to try logging out. }).done(); }, diff --git a/src/components/views/dialogs/SessionRestoreErrorDialog.js b/src/components/views/dialogs/SessionRestoreErrorDialog.js index 401550043b..f101381ebf 100644 --- a/src/components/views/dialogs/SessionRestoreErrorDialog.js +++ b/src/components/views/dialogs/SessionRestoreErrorDialog.js @@ -1,5 +1,6 @@ /* Copyright 2017 Vector Creations Ltd +Copyright 2018 New Vector Ltd Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. From 75ab618c054e8a12ee064c15a493388c8d8eac50 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 14:20:09 +0100 Subject: [PATCH 4/9] Fix variable scopes --- src/Lifecycle.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Lifecycle.js b/src/Lifecycle.js index 6c35c6ed06..7e8107c8b7 100644 --- a/src/Lifecycle.js +++ b/src/Lifecycle.js @@ -66,12 +66,13 @@ import sdk from './index'; * failed. */ export function loadSession(opts) { + let enableGuest = opts.enableGuest || false; + const guestHsUrl = opts.guestHsUrl; + const guestIsUrl = opts.guestIsUrl; + const fragmentQueryParams = opts.fragmentQueryParams || {}; + const defaultDeviceDisplayName = opts.defaultDeviceDisplayName; + return new Promise.resolve().then(() => { - const fragmentQueryParams = opts.fragmentQueryParams || {}; - let enableGuest = opts.enableGuest || false; - const guestHsUrl = opts.guestHsUrl; - const guestIsUrl = opts.guestIsUrl; - const defaultDeviceDisplayName = opts.defaultDeviceDisplayName; if (!guestHsUrl) { console.warn("Cannot enable guest access: can't determine HS URL to use"); From 2cc50d35c680f036f54b3b61dd5f18d19563a9b8 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 15:06:36 +0100 Subject: [PATCH 5/9] Lint --- src/Lifecycle.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Lifecycle.js b/src/Lifecycle.js index 7e8107c8b7..a22a5aeebd 100644 --- a/src/Lifecycle.js +++ b/src/Lifecycle.js @@ -72,8 +72,7 @@ export function loadSession(opts) { const fragmentQueryParams = opts.fragmentQueryParams || {}; const defaultDeviceDisplayName = opts.defaultDeviceDisplayName; - return new Promise.resolve().then(() => { - + return Promise.resolve().then(() => { if (!guestHsUrl) { console.warn("Cannot enable guest access: can't determine HS URL to use"); enableGuest = false; From fed74646b0eb8b6fa5cc55fff92a613810714e64 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 17:49:53 +0100 Subject: [PATCH 6/9] Rewrite to use async / await --- src/Lifecycle.js | 84 +++++++++++++++++++++++------------------------- 1 file changed, 41 insertions(+), 43 deletions(-) diff --git a/src/Lifecycle.js b/src/Lifecycle.js index a22a5aeebd..7378e982ef 100644 --- a/src/Lifecycle.js +++ b/src/Lifecycle.js @@ -65,14 +65,14 @@ import sdk from './index'; * Resolves to `true` if we ended up starting a session, or `false` if we * failed. */ -export function loadSession(opts) { - let enableGuest = opts.enableGuest || false; - const guestHsUrl = opts.guestHsUrl; - const guestIsUrl = opts.guestIsUrl; - const fragmentQueryParams = opts.fragmentQueryParams || {}; - const defaultDeviceDisplayName = opts.defaultDeviceDisplayName; +export async function loadSession(opts) { + try { + let enableGuest = opts.enableGuest || false; + const guestHsUrl = opts.guestHsUrl; + const guestIsUrl = opts.guestIsUrl; + const fragmentQueryParams = opts.fragmentQueryParams || {}; + const defaultDeviceDisplayName = opts.defaultDeviceDisplayName; - return Promise.resolve().then(() => { if (!guestHsUrl) { console.warn("Cannot enable guest access: can't determine HS URL to use"); enableGuest = false; @@ -91,8 +91,7 @@ export function loadSession(opts) { guest: true, }, true).then(() => true); } - return _restoreFromLocalStorage(); - }).then((success) => { + const success = await _restoreFromLocalStorage(); if (success) { return true; } @@ -103,9 +102,9 @@ export function loadSession(opts) { // fall back to login screen return false; - }).catch((e) => { + } catch (e) { return _handleLoadSessionFailure(e); - }); + } } /** @@ -199,40 +198,39 @@ function _registerAsGuest(hsUrl, isUrl, defaultDeviceDisplayName) { // The plan is to gradually move the localStorage access done here into // SessionStore to avoid bugs where the view becomes out-of-sync with // localStorage (e.g. teamToken, isGuest etc.) -function _restoreFromLocalStorage() { - return Promise.resolve().then(() => { - if (!localStorage) { - return Promise.resolve(false); - } - const hsUrl = localStorage.getItem("mx_hs_url"); - const isUrl = localStorage.getItem("mx_is_url") || 'https://matrix.org'; - const accessToken = localStorage.getItem("mx_access_token"); - const userId = localStorage.getItem("mx_user_id"); - const deviceId = localStorage.getItem("mx_device_id"); +async function _restoreFromLocalStorage() { + if (!localStorage) { + return false; + } + const hsUrl = localStorage.getItem("mx_hs_url"); + const isUrl = localStorage.getItem("mx_is_url") || 'https://matrix.org'; + const accessToken = localStorage.getItem("mx_access_token"); + const userId = localStorage.getItem("mx_user_id"); + const deviceId = localStorage.getItem("mx_device_id"); - let isGuest; - if (localStorage.getItem("mx_is_guest") !== null) { - isGuest = localStorage.getItem("mx_is_guest") === "true"; - } else { - // legacy key name - isGuest = localStorage.getItem("matrix-is-guest") === "true"; - } + let isGuest; + if (localStorage.getItem("mx_is_guest") !== null) { + isGuest = localStorage.getItem("mx_is_guest") === "true"; + } else { + // legacy key name + isGuest = localStorage.getItem("matrix-is-guest") === "true"; + } - if (accessToken && userId && hsUrl) { - console.log(`Restoring session for ${userId}`); - return _doSetLoggedIn({ - userId: userId, - deviceId: deviceId, - accessToken: accessToken, - homeserverUrl: hsUrl, - identityServerUrl: isUrl, - guest: isGuest, - }, false).then(() => true); - } else { - console.log("No previous session found."); - return Promise.resolve(false); - } - }); + if (accessToken && userId && hsUrl) { + console.log(`Restoring session for ${userId}`); + await _doSetLoggedIn({ + userId: userId, + deviceId: deviceId, + accessToken: accessToken, + homeserverUrl: hsUrl, + identityServerUrl: isUrl, + guest: isGuest, + }, false); + return true; + } else { + console.log("No previous session found."); + return false; + } } function _handleLoadSessionFailure(e) { From 0c309c88add295798689fbab993d326bd1549cf0 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 17:52:25 +0100 Subject: [PATCH 7/9] Bluebird has no need for your .done() --- src/components/structures/MatrixChat.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index cdeb99ef53..8df46d2f7c 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -360,7 +360,7 @@ export default React.createClass({ // Note we don't catch errors from this: we catch everything within // loadSession as there's logic there to ask the user if they want // to try logging out. - }).done(); + }); }, componentWillUnmount: function() { From d3c368e19fe97d8e429fcb8cef0762638ff1f89a Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 17:53:11 +0100 Subject: [PATCH 8/9] typo --- src/components/views/dialogs/BaseDialog.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/dialogs/BaseDialog.js b/src/components/views/dialogs/BaseDialog.js index 7959e7cb10..54623e6510 100644 --- a/src/components/views/dialogs/BaseDialog.js +++ b/src/components/views/dialogs/BaseDialog.js @@ -36,7 +36,7 @@ export default React.createClass({ propTypes: { // onFinished callback to call when Escape is pressed - // Take a boolean which is true is the dialog was dismissed + // Take a boolean which is true if the dialog was dismissed // with a positive / confirm action or false if it was // cancelled (from BaseDialog, this is always false). onFinished: PropTypes.func.isRequired, From 873993a7cae2ceae8a3207bb9cf6cc10dbf78a38 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 27 Apr 2018 17:56:33 +0100 Subject: [PATCH 9/9] Clarify, hopefully --- src/components/views/dialogs/BaseDialog.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/dialogs/BaseDialog.js b/src/components/views/dialogs/BaseDialog.js index 54623e6510..51d9180885 100644 --- a/src/components/views/dialogs/BaseDialog.js +++ b/src/components/views/dialogs/BaseDialog.js @@ -38,7 +38,7 @@ export default React.createClass({ // onFinished callback to call when Escape is pressed // Take a boolean which is true if the dialog was dismissed // with a positive / confirm action or false if it was - // cancelled (from BaseDialog, this is always false). + // cancelled (BaseDialog itself only calls this with false). onFinished: PropTypes.func.isRequired, // called when a key is pressed