Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent getTokenSilently to be called concurrently #125

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 34 additions & 6 deletions __tests__/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ import {
urlDecodeB64
} from '../src/utils';
import { DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS } from '../src/constants';
import { InternalError } from '../src/errors';

(<any>global).TextEncoder = TextEncoder;
const TIMEOUT_ERROR = { error: 'timeout', error_description: 'Timeout' };

describe('utils', () => {
describe('getUniqueScopes', () => {
Expand Down Expand Up @@ -244,7 +246,6 @@ describe('utils', () => {
});
});
describe('runPopup', () => {
const TIMEOUT_ERROR = { error: 'timeout', error_description: 'Timeout' };
const setup = customMessage => {
const popup = {
location: { href: '' },
Expand Down Expand Up @@ -347,11 +348,11 @@ describe('utils', () => {
});
});
describe('runIframe', () => {
const TIMEOUT_ERROR = { error: 'timeout', error_description: 'Timeout' };
const setup = customMessage => {
const iframe = {
setAttribute: jest.fn(),
style: { display: '' }
style: { display: '' },
id: ''
};
const url = 'https://authorize.com';
const origin =
Expand All @@ -365,6 +366,13 @@ describe('utils', () => {
expect(type).toBe('iframe');
return iframe;
});
window.document.getElementById = <any>jest
.fn()
.mockImplementationOnce(id => undefined)
.mockImplementationOnce(id => {
expect(id).toBe('a0-spajs-iframe');
return iframe;
});
window.document.body.appendChild = jest.fn();
window.document.body.removeChild = jest.fn();
return { iframe, url, origin };
Expand All @@ -382,7 +390,6 @@ describe('utils', () => {
const { iframe, url } = setup(message);
await runIframe(url, origin);

expect(message.source.close).toHaveBeenCalled();
expect(window.document.body.appendChild).toHaveBeenCalledWith(iframe);
expect(window.document.body.removeChild).toHaveBeenCalledWith(iframe);
expect(iframe.setAttribute.mock.calls).toMatchObject([
Expand All @@ -391,6 +398,29 @@ describe('utils', () => {
['src', url]
]);
expect(iframe.style.display).toBe('none');
expect(iframe.id).toBe('a0-spajs-iframe');
});
it('throws an error when the iframe already exists', async () => {
const origin = 'https://origin.com';
const { iframe, url } = setup('');

//the mock returns undefined in the first time
//and an object in the second time, so call it
//once here so when the method runs, it will
//return the object and throw
window.document.getElementById('foobar');

try {
await runIframe(url, origin);
} catch (error) {
expect(error).toBeInstanceOf(InternalError);
expect(error.error_description).toBe(
"`getTokenSilently` can only be called once at a time. Check your code to make sure you're not calling it multiple times."
);
expect(window.document.body.appendChild).not.toHaveBeenCalledWith(
iframe
);
}
});
describe('with invalid messages', () => {
[
Expand Down Expand Up @@ -434,7 +464,6 @@ describe('utils', () => {
await expect(runIframe(url, origin)).resolves.toMatchObject(
message.data.response
);
expect(message.source.close).toHaveBeenCalled();
expect(window.document.body.removeChild).toHaveBeenCalledWith(iframe);
});
it('returns authorization error message', async () => {
Expand All @@ -451,7 +480,6 @@ describe('utils', () => {
await expect(runIframe(url, origin)).rejects.toMatchObject(
message.data.response
);
expect(message.source.close).toHaveBeenCalled();
expect(window.document.body.removeChild).toHaveBeenCalledWith(iframe);
});
it('times out after 60s', async () => {
Expand Down
9 changes: 9 additions & 0 deletions src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,12 @@ export class AuthenticationError extends Error {
Object.setPrototypeOf(this, AuthenticationError.prototype);
}
}

export class InternalError extends Error {
public error = 'internal_error';
constructor(public error_description: string = 'Internal Error') {
super(error_description);
//https://github.com/Microsoft/TypeScript-wiki/blob/master/Breaking-Changes.md#extending-built-ins-like-error-array-and-map-may-no-longer-work
Object.setPrototypeOf(this, InternalError.prototype);
}
}
18 changes: 15 additions & 3 deletions src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ import * as qs from 'qss';
import fetch from 'unfetch';

import { DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS } from './constants';
import { InternalError } from './errors';

const TIMEOUT_ERROR = { error: 'timeout', error_description: 'Timeout' };

const dedupe = arr => arr.filter((x, i) => arr.indexOf(x) === i);

const TIMEOUT_ERROR = { error: 'timeout', error_description: 'Timeout' };
export const getUniqueScopes = (...scopes: string[]) => {
const scopeString = scopes.filter(Boolean).join();
return dedupe(scopeString.replace(/\s/g, ',').split(','))
Expand All @@ -22,11 +24,18 @@ export const parseQueryResult = (hash: string) => {
};

export const runIframe = (authorizeUrl: string, eventOrigin: string) => {
const IFRAME_ID = 'a0-spajs-iframe';
if (document.getElementById(IFRAME_ID)) {
throw new InternalError(
"`getTokenSilently` can only be called once at a time. Check your code to make sure you're not calling it multiple times."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when in this function's code are you calling twice getTokenSilently? if the answer is "nowhere" I think this error should be different. Even the tests mention "throws an error when the iframe already exists". Then why is this error talking about the specific use case of calling getTokenSilently multiple times? That's out of this method's scope IMO. Maybe the caller should be checking that and re-throwing a similar error up.

);
}
return new Promise<AuthenticationResult>((res, rej) => {
var iframe = window.document.createElement('iframe');
iframe.setAttribute('width', '0');
iframe.setAttribute('height', '0');
iframe.style.display = 'none';
iframe.id = IFRAME_ID;

const timeoutSetTimeoutId = setTimeout(() => {
rej(TIMEOUT_ERROR);
Expand All @@ -36,11 +45,14 @@ export const runIframe = (authorizeUrl: string, eventOrigin: string) => {
const iframeEventHandler = function(e: MessageEvent) {
if (e.origin != eventOrigin) return;
if (!e.data || e.data.type !== 'authorization_response') return;
(<any>e.source).close();
luisrudge marked this conversation as resolved.
Show resolved Hide resolved

e.data.response.error ? rej(e.data.response) : res(e.data.response);
clearTimeout(timeoutSetTimeoutId);
window.removeEventListener('message', iframeEventHandler, false);
window.document.body.removeChild(iframe);

try {
window.document.body.removeChild(document.getElementById(IFRAME_ID));
} catch (error) {}
luisrudge marked this conversation as resolved.
Show resolved Hide resolved
};
window.addEventListener('message', iframeEventHandler, false);
window.document.body.appendChild(iframe);
Expand Down