Repository: auth0/auth0-spa-js (ts)

Issue:
loginWithPopup is blocked by firefox since version 1.6.5
### Describe the problem
Using loginWithPopup in firefox in an onclick works in 1.6.4. However, in 1.6.5 the same code now triggers firefox pop blocker. 

I have done some preliminary investigation and I believe the issue was introduced in [this commit](https://github.com/auth0/auth0-spa-js/pull/368/commits/6665dc071e7e27d480ef2b7d91673741fe571d5e).

Prior to this, the code would call window.open during the asynchronous part of the function call - then later, it would asynchronously set the URL of the pop up. This is the correct way to handle this. However, after the refactor the popup was moved to the asynchronous code - this causes Firefox to block it with its popup blocker. In Girefox if you try to open a window asynchronously it will [no longer be treated as a user action ](https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/User_actions). 

### What was the expected behavior?

The window should open without being blocked by the popup blocker.

### Reproduction


**Can the behavior be reproduced using the [SPA SDK Playground](https://github.com/auth0/auth0-spa-js/blob/master/DEVELOPMENT.md#the-sdk-playground)?**

If so, provide steps:

Run the playground, click login with popup in firefox. Get a banner showing "firefox prevented this site from opening a pop up window". Console shows "Uncaught (in promise) Error: Could not open popup"

### Environment

> Please provide the following:

- **Version of `auth0-spa-js` used:**
- Listed above - any version after 1.6.4 has this issue
- 
- **Which browsers have you tested in?**
- Issue occurs in firefox
- 
- **Which framework are you using, if applicable (Angular, React, etc):**
- Vue
- 
- **Other modules/plugins/libraries that might be involved:**
None
Cannot login if cookies are blocked using refresh tokens and local cache storage
Using the latest version 1.14.0. When I run loginwithpop up using social login (google), refresh tokens and cache set to localstorage. The login fails ,on second attempt I get this error in Safari:

[Error] TypeError: null is not an object (evaluating 't.organization')

	_callee$ (app.js:16429)
	l (chunk-vendors.js:27481:5863)
	(anonymous function) (chunk-vendors.js:27481:5648)
	asyncGeneratorStep (chunk-vendors.js:131)
	_throw (chunk-vendors.js:157)
	promiseReactionJob

In Firefox, if I set it to block all third party cookies it shows as:

TypeError: t is null
    node_modules auth0/auth0-spa-js/dist/auth0-spa-js.production.esm.js/</Vo</t.prototype.loginWithPopup/</<@http://localhost:8080/js/chunk-vendors.js:32
    s http://localhost:8080/js/chunk-vendors.js:32
    s http://localhost:8080/js/chunk-vendors.js:32
    c http://localhost:8080/js/chunk-vendors.js:32
    promise callback*a http://localhost:8080/js/chunk-vendors.js:32
    c http://localhost:8080/js/chunk-vendors.js:32
    promise callback*a http://localhost:8080/js/chunk-vendors.js:32
    c http://localhost:8080/js/chunk-vendors.js:32
    promise callback*a http://localhost:8080/js/chunk-vendors.js:32
    r http://localhost:8080/js/chunk-vendors.js:32
    r http://localhost:8080/js/chunk-vendors.js:32
    node_modules auth0/auth0-spa-js/dist/auth0-spa-js.production.esm.js/</Vo</t.prototype.loginWithPopup@http://localhost:8080/js/chunk-vendors.js:32
    _callee$ http://localhost:8080/js/app.js:17333
    l http://localhost:8080/js/chunk-vendors.js:36663
    _invoke http://localhost:8080/js/chunk-vendors.js:36663
    t http://localhost:8080/js/chunk-vendors.js:36663
    asyncGeneratorStep http://localhost:8080/js/chunk-vendors.js:131
    _next http://localhost:8080/js/chunk-vendors.js:153
    _asyncToGenerator http://localhost:8080/js/chunk-vendors.js:160
    _asyncToGenerator http://localhost:8080/js/chunk-vendors.js:149
    loginWithPopup http://localhost:8080/js/app.js:17364
    _callee$ http://localhost:8080/js/app.js:1220
    l http://localhost:8080/js/chunk-vendors.js:36663
    _invoke http://localhost:8080/js/chunk-vendors.js:36663
    t http://localhost:8080/js/chunk-vendors.js:36663
    asyncGeneratorStep http://localhost:8080/js/chunk-vendors.js:131
    _next http://localhost:8080/js/chunk-vendors.js:153
    _asyncToGenerator http://localhost:8080/js/chunk-vendors.js:160
    _asyncToGenerator http://localhost:8080/js/chunk-vendors.js:149
    loginClick http://localhost:8080/js/app.js:1231
    click http://localhost:8080/js/app.js:7562
    invokeWithErrorHandling http://localhost:8080/js/chunk-vendors.js:38806
    invoker http://localhost:8080/js/chunk-vendors.js:39131
    invokeWithErrorHandling http://localhost:8080/js/chunk-vendors.js:38806
    $emit http://localhost:8080/js/chunk-vendors.js:40835
    click http://localhost:8080/js/chunk-vendors.js:47527
    invokeWithErrorHandling http://localhost:8080/js/chunk-vendors.js:38806
    invoker http://localhost:8080/js/chunk-vendors.js:39131
    _wrapper http://localhost:8080/js/chunk-vendors.js:43860

How can I troubleshoot this? Am I doing something wrong? 

My main goal is to allow logins using safari. All the posts on this issue says to use refresh tokens  and set cache to localstorage, however that doesn't seem to be working with Google OAuth login.

Pull request description:
Fix popup blocker showing for loginWithPopup in Firefox & Safari
This PR moves the creation of the popup window in `loginWithPopup` outside of an async context, so that browsers like Firefox and Safari don't attempt to block the popup as it will now come directly from a user interaction event.

It achieves this by moving popup creation into `loginWithPopup` itself instead of inside `runPopup`.

This PR also fixes an issue where calling `loginWithPopup(null)` would cause the method to crash when it tried to access `options.organization`. It does this by making the parameters optional, instead of providing default values (which are now assigned inside the method).

Fixes #726
Fixes #728


Patch:
diff --git a/src/Auth0Client.ts b/src/Auth0Client.ts
index 77352b9..8fde49a 100644
--- a/src/Auth0Client.ts
+++ b/src/Auth0Client.ts
@@ -9,7 +9,8 @@ import {
   runIframe,
   sha256,
   bufferToBase64UrlEncoded,
-  validateCrypto
+  validateCrypto,
+  openPopup
 } from './utils';
 
 import { oauthToken, TokenEndpointResponse } from './api';
@@ -335,9 +336,16 @@ export default class Auth0Client {
    * @param config
    */
   public async loginWithPopup(
-    options: PopupLoginOptions = {},
-    config: PopupConfigOptions = {}
+    options?: PopupLoginOptions,
+    config?: PopupConfigOptions
   ) {
+    options = options || {};
+    config = config || {};
+
+    if (!config.popup) {
+      config.popup = openPopup('');
+    }
+
     const { ...authorizeOptions } = options;
     const stateIn = encode(createRandomString());
     const nonceIn = encode(createRandomString());
@@ -358,7 +366,9 @@ export default class Auth0Client {
       response_mode: 'web_message'
     });
 
-    const codeResult = await runPopup(url, {
+    config.popup.location.href = url;
+
+    const codeResult = await runPopup({
       ...config,
       timeoutInSeconds:
         config.timeoutInSeconds ||
@@ -420,7 +430,7 @@ export default class Auth0Client {
    * (the SDK stores a corresponding ID Token with every Access Token, and uses the
    * scope and audience to look up the ID Token)
    *
-   * @typeparam TUser The type to return, has to extend {@link User}. 
+   * @typeparam TUser The type to return, has to extend {@link User}.
    * @param options
    */
   public async getUser<TUser extends User>(
@@ -453,7 +463,9 @@ export default class Auth0Client {
    *
    * @param options
    */
-  public async getIdTokenClaims(options: GetIdTokenClaimsOptions = {}): Promise<IdToken> {
+  public async getIdTokenClaims(
+    options: GetIdTokenClaimsOptions = {}
+  ): Promise<IdToken> {
     const audience = options.audience || this.options.audience || 'default';
     const scope = getUniqueScopes(this.defaultScope, this.scope, options.scope);
 
@@ -829,8 +841,8 @@ export default class Auth0Client {
       nonceIn,
       code_challenge,
       options.redirect_uri ||
-      this.options.redirect_uri ||
-      window.location.origin
+        this.options.redirect_uri ||
+        window.location.origin
     );
 
     const url = this._authorizeUrl({
diff --git a/src/utils.ts b/src/utils.ts
index 49307d5..1498d37 100644
--- a/src/utils.ts
+++ b/src/utils.ts
@@ -80,7 +80,7 @@ export const runIframe = (
   });
 };
 
-const openPopup = (url: string) => {
+export const openPopup = (url: string) => {
   const width = 400;
   const height = 600;
   const left = window.screenX + (window.innerWidth - width) / 2;
@@ -93,24 +93,12 @@ const openPopup = (url: string) => {
   );
 };
 
-export const runPopup = (authorizeUrl: string, config: PopupConfigOptions) => {
-  let popup = config.popup;
-
-  if (popup) {
-    popup.location.href = authorizeUrl;
-  } else {
-    popup = openPopup(authorizeUrl);
-  }
-
-  if (!popup) {
-    throw new Error('Could not open popup');
-  }
-
+export const runPopup = (config: PopupConfigOptions) => {
   return new Promise<AuthenticationResult>((resolve, reject) => {
     let popupEventListener: EventListenerOrEventListenerObject;
 
     const timeoutId = setTimeout(() => {
-      reject(new PopupTimeoutError(popup));
+      reject(new PopupTimeoutError(config.popup));
       window.removeEventListener('message', popupEventListener, false);
     }, (config.timeoutInSeconds || DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS) * 1000);
 
@@ -121,7 +109,7 @@ export const runPopup = (authorizeUrl: string, config: PopupConfigOptions) => {
 
       clearTimeout(timeoutId);
       window.removeEventListener('message', popupEventListener, false);
-      popup.close();
+      config.popup.close();
 
       if (e.data.response.error) {
         return reject(GenericError.fromPayload(e.data.response));


Test patch:
diff --git a/__tests__/Auth0Client/helpers.ts b/__tests__/Auth0Client/helpers.ts
index b5023be..246007d 100644
--- a/__tests__/Auth0Client/helpers.ts
+++ b/__tests__/Auth0Client/helpers.ts
@@ -231,7 +231,10 @@ export const setupMessageEventLister = (
   });
 
   mockWindow.open.mockReturnValue({
-    close: () => {}
+    close: () => {},
+    location: {
+      href: ''
+    }
   });
 };
 
diff --git a/__tests__/Auth0Client/loginWithPopup.test.ts b/__tests__/Auth0Client/loginWithPopup.test.ts
index b1cddee..6f1cde1 100644
--- a/__tests__/Auth0Client/loginWithPopup.test.ts
+++ b/__tests__/Auth0Client/loginWithPopup.test.ts
@@ -33,7 +33,6 @@ import {
   DEFAULT_AUTH0_CLIENT,
   DEFAULT_POPUP_CONFIG_OPTIONS
 } from '../../src/constants';
-import version from '../../src/version';
 
 jest.mock('unfetch');
 jest.mock('es-cookie');
@@ -76,6 +75,7 @@ describe('Auth0Client', () => {
 
     mockWindow.open = jest.fn();
     mockWindow.addEventListener = jest.fn();
+
     mockWindow.crypto = {
       subtle: {
         digest: () => 'foo'
@@ -99,6 +99,9 @@ describe('Auth0Client', () => {
   describe('loginWithPopup', () => {
     it('should log the user in and get the user and claims', async () => {
       const auth0 = setup({ scope: 'foo' });
+
+      mockWindow.open.mockReturnValue({ hello: 'world' });
+
       await loginWithPopup(auth0);
 
       const expectedUser = { sub: 'me' };
@@ -142,7 +145,9 @@ describe('Auth0Client', () => {
 
       await loginWithPopup(auth0);
 
-      const [[url]] = (<jest.Mock>mockWindow.open).mock.calls;
+      // prettier-ignore
+      const url = (utils.runPopup as jest.Mock).mock.calls[0][0].popup.location.href;
+
       assertUrlEquals(url, 'auth0_domain', '/authorize', {
         state: TEST_STATE,
         nonce: TEST_NONCE
@@ -154,7 +159,9 @@ describe('Auth0Client', () => {
 
       await loginWithPopup(auth0);
 
-      const [[url]] = (<jest.Mock>mockWindow.open).mock.calls;
+      // prettier-ignore
+      const url = (utils.runPopup as jest.Mock).mock.calls[0][0].popup.location.href;
+
       assertUrlEquals(url, 'auth0_domain', '/authorize', {
         code_challenge: TEST_CODE_CHALLENGE,
         code_challenge_method: 'S256'
@@ -170,7 +177,10 @@ describe('Auth0Client', () => {
       });
 
       expect(mockWindow.open).toHaveBeenCalled();
-      const [[url]] = (<jest.Mock>mockWindow.open).mock.calls;
+
+      // prettier-ignore
+      const url = (utils.runPopup as jest.Mock).mock.calls[0][0].popup.location.href;
+
       assertUrlEquals(url, 'auth0_domain', '/authorize', {
         redirect_uri: 'http://localhost',
         client_id: TEST_CLIENT_ID,
@@ -195,7 +205,10 @@ describe('Auth0Client', () => {
       });
 
       expect(mockWindow.open).toHaveBeenCalled();
-      const [[url]] = (<jest.Mock>mockWindow.open).mock.calls;
+
+      // prettier-ignore
+      const url = (utils.runPopup as jest.Mock).mock.calls[0][0].popup.location.href;
+
       assertUrlEquals(url, TEST_DOMAIN, '/authorize', {
         redirect_uri: TEST_REDIRECT_URI,
         client_id: TEST_CLIENT_ID,
@@ -215,8 +228,12 @@ describe('Auth0Client', () => {
       const auth0 = setup({
         useRefreshTokens: true
       });
+
       await loginWithPopup(auth0);
-      const [[url]] = (<jest.Mock>mockWindow.open).mock.calls;
+
+      // prettier-ignore
+      const url = (utils.runPopup as jest.Mock).mock.calls[0][0].popup.location.href;
+
       assertUrlEquals(url, TEST_DOMAIN, '/authorize', {
         scope: `${TEST_SCOPES} offline_access`
       });
@@ -228,7 +245,10 @@ describe('Auth0Client', () => {
         redirect_uri
       });
       await loginWithPopup(auth0);
-      const [[url]] = (<jest.Mock>mockWindow.open).mock.calls;
+
+      // prettier-ignore
+      const url = (utils.runPopup as jest.Mock).mock.calls[0][0].popup.location.href;
+
       assertUrlEquals(url, TEST_DOMAIN, '/authorize', {
         redirect_uri
       });
@@ -236,10 +256,9 @@ describe('Auth0Client', () => {
 
     it('should log the user in with a popup and get the token', async () => {
       const auth0 = setup();
-
       await loginWithPopup(auth0);
-
       expect(mockWindow.open).toHaveBeenCalled();
+
       assertPost(
         'https://auth0_domain/oauth/token',
         {
@@ -260,10 +279,10 @@ describe('Auth0Client', () => {
 
       await loginWithPopup(auth0);
 
-      expect(utils.runPopup).toHaveBeenCalledWith(
-        expect.any(String),
-        DEFAULT_POPUP_CONFIG_OPTIONS
-      );
+      expect(utils.runPopup).toHaveBeenCalledWith({
+        ...DEFAULT_POPUP_CONFIG_OPTIONS,
+        popup: expect.anything()
+      });
     });
 
     it('should be able to provide custom config', async () => {
@@ -271,8 +290,9 @@ describe('Auth0Client', () => {
 
       await loginWithPopup(auth0, {}, { timeoutInSeconds: 3 });
 
-      expect(utils.runPopup).toHaveBeenCalledWith(expect.any(String), {
-        timeoutInSeconds: 3
+      expect(utils.runPopup).toHaveBeenCalledWith({
+        timeoutInSeconds: 3,
+        popup: expect.anything()
       });
     });
 
@@ -345,7 +365,10 @@ describe('Auth0Client', () => {
       await loginWithPopup(auth0);
 
       expect(mockWindow.open).toHaveBeenCalled();
-      const [[url]] = (<jest.Mock>mockWindow.open).mock.calls;
+
+      // prettier-ignore
+      const url = (utils.runPopup as jest.Mock).mock.calls[0][0].popup.location.href;
+
       assertUrlEquals(url, TEST_DOMAIN, '/authorize', {
         auth0Client: btoa(JSON.stringify(auth0Client))
       });
diff --git a/__tests__/utils.test.ts b/__tests__/utils.test.ts
index c19726a..bdc7de9 100644
--- a/__tests__/utils.test.ts
+++ b/__tests__/utils.test.ts
@@ -268,7 +268,7 @@ describe('utils', () => {
               jest.runAllTimers();
             }, 10);
             jest.useFakeTimers();
-            await expect(runPopup(url, { popup })).rejects.toMatchObject(
+            await expect(runPopup({ popup })).rejects.toMatchObject(
               TIMEOUT_ERROR
             );
             jest.useRealTimers();
@@ -287,7 +287,7 @@ describe('utils', () => {
 
       const { popup, url } = setup(message);
 
-      await expect(runPopup(url, { popup })).resolves.toMatchObject(
+      await expect(runPopup({ popup })).resolves.toMatchObject(
         message.data.response
       );
 
@@ -308,7 +308,7 @@ describe('utils', () => {
 
       const { popup, url } = setup(message);
 
-      await expect(runPopup(url, { popup })).rejects.toMatchObject({
+      await expect(runPopup({ popup })).rejects.toMatchObject({
         ...message.data.response,
         message: 'error_description'
       });
@@ -334,7 +334,7 @@ describe('utils', () => {
       jest.useFakeTimers();
 
       await expect(
-        runPopup(url, {
+        runPopup({
           timeoutInSeconds: seconds,
           popup
         })
@@ -357,35 +357,10 @@ describe('utils', () => {
 
       jest.useFakeTimers();
 
-      await expect(runPopup(url, { popup })).rejects.toMatchObject(
-        TIMEOUT_ERROR
-      );
+      await expect(runPopup({ popup })).rejects.toMatchObject(TIMEOUT_ERROR);
 
       jest.useRealTimers();
     });
-
-    it('creates and uses a popup window if none was given', async () => {
-      const message = {
-        data: {
-          type: 'authorization_response',
-          response: { id_token: 'id_token' }
-        }
-      };
-
-      const { popup, url } = setup(message);
-      const oldOpenFn = window.open;
-
-      window.open = <any>jest.fn(() => popup);
-
-      await expect(runPopup(url, {})).resolves.toMatchObject(
-        message.data.response
-      );
-
-      expect(popup.location.href).toBe(url);
-      expect(popup.close).toHaveBeenCalled();
-
-      window.open = oldOpenFn;
-    });
   });
   describe('runIframe', () => {
     const TIMEOUT_ERROR = {