From 2b6eb87252af00ddabc2d79cedbffebed7ee9b02 Mon Sep 17 00:00:00 2001 From: Dennis Snell Date: Sun, 13 Oct 2019 16:31:51 -0700 Subject: [PATCH] Fix: Broken oAuth flow (#1627) We have been experiencing problems when trying to login with the WordPress.com signin. Something appears to have changed in Electron such that the older versions of the app still work but newer versions are failing. In this patch we're rewriting the authentication flow to simplify it and prepare ourselves for better handling of the failure cases. In production we are seeing strange behaviors on failure and some on success: unending re-requests to `simplenote://auth` which trigger full CPU load; and no response after authentication. After this patch we should be able to wrangle in errors and add a timeout to better communicate when things are failing. Additionally, the unending loop should be closed due to a replacement of the old network intercept code with a single simplified model. We have also been sharing sessions between the main window and the auth window and also sharing sessions between teach time the auth window appears. This leads to leaked cookies and can result in confusing flows, largely because of the shared cookies. In this patch we're creating a new `Session` for the auth window every time we open it. By not including `persist:` in the "partition" name we're making sure it only exists in memory. By introducing randomness into its name we're making sure we don't share the same session from one auth attempt to the next. By freeing the window after close we're making sure we don't leak memory. Previously we were able to open the auth window after closing it and instead of logging in again it would open to the "Accept/Deny" view. After this change it requires logging in on every attempt. This will likely be more frustrating but much safer than the previous behavior. --- RELEASE-NOTES.txt | 6 +++ lib/auth/index.jsx | 121 +++++++++++++++++++++------------------------ 2 files changed, 61 insertions(+), 66 deletions(-) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index b5c9348bf..9b0eebbe8 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -1,5 +1,11 @@ # Changelog +## Future release + +### Fixes + + - Rework WordPress.com signin to prevent infinite looping and login failures [#1627](https://github.com/Automattic/simplenote-electron/pull/1627) + ## [v1.9.0] ### Enhancements diff --git a/lib/auth/index.jsx b/lib/auth/index.jsx index c01e2c19a..84524b5a2 100644 --- a/lib/auth/index.jsx +++ b/lib/auth/index.jsx @@ -216,84 +216,73 @@ export class Auth extends Component { }; setupAuthWindow = () => { - const remote = __non_webpack_require__('electron').remote; // eslint-disable-line no-undef - const BrowserWindow = remote.BrowserWindow; - const protocol = remote.protocol; + // eslint-disable-next-line no-undef + const { BrowserWindow, session } = __non_webpack_require__( + 'electron' + ).remote; + this.authWindow = new BrowserWindow({ width: 640, height: 640, show: false, webPreferences: { nodeIntegration: false, + session: session.fromPartition(`fresh-session-${Math.random()}`), }, }); - // Register simplenote:// protocol - protocol.registerHttpProtocol('simplenote', req => { - this.authWindow.loadURL(req.url); + this.authWindow.on('closed', () => { + // make sure to release this from memory + this.authWindow = null; }); - this.authWindow.webContents.on('will-navigate', (event, url) => - this.onBrowserNavigate(url) - ); - - this.authWindow.webContents.on( - 'did-get-redirect-request', - (event, oldUrl, newUrl) => this.onBrowserNavigate(newUrl) - ); - }; - - onBrowserNavigate = url => { - try { - this.authenticateWithUrl(new URL(url)); - } catch (error) { - // Do nothing if Url was invalid - } - }; - - authenticateWithUrl = url => { - // Bail out if the url is not the simplenote protocol - if (url.protocol !== 'simplenote:') { - return; - } - - const { authorizeUserWithToken, saveWPToken } = this.props; - const params = url.searchParams; - - // Display an error message if authorization failed. - if (params.get('error')) { - switch (params.get('code')) { - case '1': - return this.authError( - 'Please activate your WordPress.com account via email and try again.' - ); - default: - return this.authError('An error was encountered while signing in.'); + this.authWindow.webContents.session.protocol.registerHttpProtocol( + 'simplenote', + (req, callback) => { + const { searchParams } = new URL(req.url); + + // cancel the request by running callback() with no parameters + // we're going to close the window and continue processing the + // information we received in args of the simplenote://auth URL + callback(); + + const errorCode = searchParams.get('error') + ? searchParams.get('code') + : false; + const authState = searchParams.get('state'); + const userEmail = searchParams.get('user'); + const simpToken = searchParams.get('token'); + const wpccToken = searchParams.get('wp_token'); + + // Display an error message if authorization failed. + switch (errorCode) { + case false: + break; + case '1': + return this.authError( + 'Please activate your WordPress.com account via email and try again.' + ); + case '2': + return this.authError( + 'Please confirm your account with the confirmation email before signing in to Simplenote.' + ); + default: + return this.authError('An error was encountered while signing in.'); + } + + this.closeAuthWindow(); + + if (authState !== this.authState) { + return; + } + + const { authorizeUserWithToken, saveWPToken } = this.props; + authorizeUserWithToken(userEmail, simpToken); + if (wpccToken) { + saveWPToken(wpccToken); + } } - } - - const userEmail = params.get('user'); - const spToken = params.get('token'); - const state = params.get('state'); - - // Sanity check on params - if (!(spToken && userEmail && state)) { - return this.closeAuthWindow(); - } - - // Verify that the state strings match - if (state !== this.authState) { - return; - } - - authorizeUserWithToken(userEmail, spToken); - - const wpToken = params.get('wp_token'); - if (wpToken) { - saveWPToken(wpToken); - } - - this.closeAuthWindow(); + ); }; authError = errorMessage => {