Skip to content

Commit

Permalink
Simplify login popup creation flow
Browse files Browse the repository at this point in the history 
The process of creating and navigating the login popup used to involve two
steps, first creating a blank window and then navigating it to the final
authorization URL. This was needed because, in Firefox, the popup window had to
be created in the same turn of the event loop as the user's click on the "Log
in" button (otherwise the popup blocker would trigger) but generating the
authorization URL involved an async "fetch" of API links.

The major browsers have now all settled on a more flexible model for allowing
popups in response to user gestures, where the popup must be opened within a
certain time window of the gesture. In practice the timeout seems to be ~1000ms
in Safari and longer than that in other browsers.

In this context we expect the async delay between the user clicking the "Log in"
button and us creating the popup to be ~0ms, since the API links should already
have been fetched at this point and so we're just fetching locally cached values.

Based on this assumption, the flow for creating the login popup window has been
simplified to create the popup window at the final URL immediately, removing the
need to open a blank window as a first step.

Simplifying the code here will make it easier to change how the popup window and
sidebar communicate, eg. to resolve an issue with the new
Cross-Origin-Opener-Policy header [1].

[1] https://github.com/hypothesis/product-backlog/issues/1333
  • Loading branch information
@robertknight
robertknight committed Mar 31, 2022
1 parent 14a1cb7 commit b111b63
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 97 deletions.
10 changes: 8 additions & 2 deletions src/sidebar/services/auth.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,9 +270,15 @@ export class AuthService extends TinyEmitter {
* then exchange for access and refresh tokens.
*/
async function login() {
const authWindow = OAuthClient.openAuthPopupWindow($window);
// Any async steps before the call to `client.authorize` must complete
// in less than ~1 second, otherwise the browser's popup blocker may block
// the popup.
//
// `oauthClient` is async in case in needs to fetch links from the API.
// This should already have happened by the time this function is called
// however, so it will just be returning a cached value.
const client = await oauthClient();
const code = await client.authorize($window, authWindow);
const code = await client.authorize($window);

// Save the auth code. It will be exchanged for an access token when the
// next API request is made.
Expand Down
10 changes: 3 additions & 7 deletions src/sidebar/services/test/auth-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ describe('AuthService', () => {
fakeClient.config = config;
return fakeClient;
};
FakeOAuthClient.openAuthPopupWindow = sinon.stub();

fakeWindow = new FakeWindow();

Expand Down Expand Up @@ -530,12 +529,9 @@ describe('AuthService', () => {
fakeSettings.services = [];
});

it('calls OAuthClient#authorize', () => {
const fakePopup = {};
FakeOAuthClient.openAuthPopupWindow.returns(fakePopup);
return auth.login().then(() => {
assert.calledWith(fakeClient.authorize, fakeWindow, fakePopup);
});
it('calls OAuthClient#authorize', async () => {
await auth.login();
assert.calledWith(fakeClient.authorize, fakeWindow);
});

it('resolves when auth completes successfully', () => {
Expand Down
70 changes: 24 additions & 46 deletions src/sidebar/util/oauth-client.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,9 @@ export class OAuthClient {
* Returns an authorization code which can be passed to `exchangeAuthCode`.
*
* @param {Window} $window - Window which will receive the auth response.
* @param {Window} authWindow - Popup window where the login prompt will be shown.
* This should be created using `openAuthPopupWindow`.
* @return {Promise<string>}
*/
authorize($window, authWindow) {
authorize($window) {
// Random state string used to check that auth messages came from the popup
// window that we opened.
//
Expand Down Expand Up @@ -160,10 +158,29 @@ export class OAuthClient {
authURL.searchParams.set('response_type', 'code');
authURL.searchParams.set('state', state);

// @ts-ignore - TS doesn't support `location = <string>`. We have to
// use this method to set the URL rather than `location.href = <string>`
// because `authWindow` is cross-origin.
authWindow.location = authURL.toString();
// In Chrome & Firefox the sizes passed to `window.open` are used for the
// viewport size. In Safari the size is used for the window size including
// title bar etc. There is enough vertical space at the bottom to allow for
// this.
//
// See https://bugs.webkit.org/show_bug.cgi?id=143678
const width = 475;
const height = 430;
const left = $window.screen.width / 2 - width / 2;
const top = $window.screen.height / 2 - height / 2;

// Generate settings for `window.open` in the required comma-separated
// key=value format.
const authWindowSettings = `left=${left},top=${top},width=${width},height=${height}`;
const authWindow = $window.open(
authURL.toString(),
'Log in to Hypothesis',
authWindowSettings
);

if (!authWindow) {
throw new Error('Failed to open login window');
}

return authResponse.then(rsp => rsp.code);
}
Expand Down Expand Up @@ -220,43 +237,4 @@ export class OAuthClient {
refreshToken: response.refresh_token,
};
}

/**
* Create and show a pop-up window for use with `OAuthClient#authorize`.
*
* This function _must_ be called in the same turn of the event loop as the
* button or link which initiates login to avoid triggering the popup blocker
* in certain browsers. See https://github.com/hypothesis/client/issues/534
* and https://github.com/hypothesis/client/issues/535.
*
* @param {Window} $window - The parent of the created window.
* @return {Window} The new popup window.
*/
static openAuthPopupWindow($window) {
// In Chrome & Firefox the sizes passed to `window.open` are used for the
// viewport size. In Safari the size is used for the window size including
// title bar etc. There is enough vertical space at the bottom to allow for
// this.
//
// See https://bugs.webkit.org/show_bug.cgi?id=143678
const width = 475;
const height = 430;
const left = $window.screen.width / 2 - width / 2;
const top = $window.screen.height / 2 - height / 2;

// Generate settings for `window.open` in the required comma-separated
// key=value format.
const authWindowSettings = `left=${left},top=${top},width=${width},height=${height}`;
const authWindow = $window.open(
'about:blank',
'Log in to Hypothesis',
authWindowSettings
);

if (!authWindow) {
throw new Error('Failed to open login window');
}

return authWindow;
}
}
79 changes: 37 additions & 42 deletions src/sidebar/util/test/oauth-client-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,29 +184,6 @@ describe('sidebar/util/oauth-client', () => {
});
});

describe('#openAuthPopupWindow', () => {
it('opens a popup window', () => {
const fakeWindow = new FakeWindow();
const popupWindow = OAuthClient.openAuthPopupWindow(fakeWindow);
assert.equal(popupWindow, fakeWindow.open.returnValues[0]);
assert.calledWith(
fakeWindow.open,
'about:blank',
'Log in to Hypothesis',
'left=274.5,top=169,width=475,height=430'
);
});

it('throws error if popup cannot be opened', () => {
const fakeWindow = new FakeWindow();
fakeWindow.open = sinon.stub().returns(null);

assert.throws(() => {
OAuthClient.openAuthPopupWindow(fakeWindow);
}, 'Failed to open login window');
});
});

describe('#authorize', () => {
let fakeWindow;

Expand All @@ -221,35 +198,53 @@ describe('sidebar/util/oauth-client', () => {
});

function authorize() {
const popupWindow = OAuthClient.openAuthPopupWindow(fakeWindow);
const authorized = client.authorize(fakeWindow, popupWindow);
return { authorized, popupWindow };
return client.authorize(fakeWindow);
}

it('navigates the popup window to the authorization URL', () => {
const { authorized, popupWindow } = authorize();
it('opens a popup window at the authorization URL', async () => {
const authorized = authorize();

const params = new URLSearchParams({
client_id: config.clientId,
origin: 'https://client.hypothes.is',
response_mode: 'web_message',
response_type: 'code',
state: 'notrandom',
});
const expectedAuthURL = `${config.authorizationEndpoint}?${params}`;

assert.calledWith(
fakeWindow.open,
expectedAuthURL,
'Log in to Hypothesis',
'left=274.5,top=169,width=475,height=430'
);

fakeWindow.sendMessage({
type: 'authorization_response',
code: 'expected-code',
state: 'notrandom',
});

return authorized.then(() => {
const params = new URLSearchParams({
client_id: config.clientId,
origin: 'https://client.hypothes.is',
response_mode: 'web_message',
response_type: 'code',
state: 'notrandom',
});
const expectedAuthUrl = `${config.authorizationEndpoint}?${params}`;
assert.equal(popupWindow.location.href, expectedAuthUrl);
});
await authorized;
});

it('rejects if popup cannot be opened', async () => {
fakeWindow.open = sinon.stub().returns(null);

let error;
try {
await authorize();
} catch (e) {
error = e;
}

assert.instanceOf(error, Error);
assert.equal(error.message, 'Failed to open login window');
});

it('resolves with an auth code if successful', () => {
const { authorized } = authorize();
const authorized = authorize();

fakeWindow.sendMessage({
type: 'authorization_response',
Expand All @@ -263,7 +258,7 @@ describe('sidebar/util/oauth-client', () => {
});

it('rejects with an error if canceled', () => {
const { authorized } = authorize();
const authorized = authorize();

fakeWindow.sendMessage({
type: 'authorization_canceled',
Expand All @@ -276,7 +271,7 @@ describe('sidebar/util/oauth-client', () => {
});

it('ignores responses with incorrect "state" values', () => {
const { authorized } = authorize();
const authorized = authorize();

fakeWindow.sendMessage({
type: 'authorization_response',
Expand Down

0 comments on commit b111b63

Please sign in to comment.