Fix popup blocker showing for loginWithPopup in Firefox & Safari

[stevehobbsdev]

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

[someone1]

I was interested in learning how you implemented this as we worked around some popup/cookie issues using custom Auth0 domains and such, and noticed one small thing that may not even be an issue.

I see that the popup is initialized and navigated to prior to a message event listener being setup. Though I doubt devices would complete the Auth0 process faster than they get around to setting up the event listener, it seemed like something a reordering of some code could fix - setting up the event listener before initiating the task that will send an event.

Just thought I'd mention it!

[stevehobbsdev]

Yes it's a good catch. Unlikely to cause a real issue but I will look at refactoring that - thanks

[shan-du]

Hi @stevehobbsdev

I've encountered an issue with getTokenWithPopup, which calls loginWithPopup.
For some users, somehow their browsers were still blocking the popup for some reason, and when they ignore or deny the popup, then config.popup will be a null object and throws a null pointer exception.

I think the code is not handling this case, see line 345 ~ 369 of Auth0Client.ts

    if (!config.popup) {
      config.popup = openPopup('');
    }

    ...

    config.popup.location.href = url; // <-- this would cause exception if popup is undefined

We had to do a string match to treat this exception in our code but that's very hacky imo, perhaps you guys could catch this error (e.g. popup being null) on your end & reject with a specific error type?

[frederikprijck]

Wouldnt it make sense for your application to ensure you don't use getTokenWithPopup when the user has popups blocked? I believe it would avoid any error...

That said, I agree we should probably throw a more explicit error in this case, but I would still try and avoid calling it when popups are blocked.

[shan-du]

@frederikprijck yep yep 💯 i'm actually trying to implement this fix exactly like you mentioned right now by doing a popup detection first before calling that method, & give user a visible notification if the popup is blocked.

but also like you said, this was def a missed case & it should be throwing an explicit error (e.g. PopupCancelledError)

[frederikprijck]

@shan-du thanks for pointing it out!