Skip to content

Commit

Permalink
Fix: Broken oAuth flow (#1627)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
dmsnell authored Oct 13, 2019
1 parent 3354b7f commit 2b6eb87
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 66 deletions.
6 changes: 6 additions & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
@@ -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
Expand Down
121 changes: 55 additions & 66 deletions lib/auth/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand Down

0 comments on commit 2b6eb87

Please sign in to comment.