Fix Dismiss AS/SF authentication sessions upon deep-link callback

[cysp]

Changes

Cancel any active ASWebAuthenticationSession of SFAuthenticationSession when completing via a deep-link callback.
Currently this scenario results in a crash due to messaging a deallocated authentication session object, as reported in #213

Technical implementation:

• create a wrapper that can hold either a ASWebAuthenticationSession or a SFAuthenticationSession, exposing their common API
• in SafariAuthenticationSession, override AuthSession's implementation of resume(_:options:), forwarding to AuthSession and, if successfully resumed, dismissing the relevant authentication session
• whilst here, also override cancel() to dismiss the authentication session

References

• Auth0 login EXC_BAD_ACCESS after callback #213

Testing

• This change adds unit test coverage
• This change has been tested on the latest version of the platform/language or why not

I couldn't see any existing test suite for this functionality so I tried to write some to cover the completion / cancellation logic but I have not had any success. 😕

To manually test this, configure an Auth0 tenant with passwordless authentication via email and attempt to sign in using the link in the email.

I haven't executed the !swift(>3.2) branches here, but I didn't think that I should just abandon them, nor remove them in this PR. 🙃

Checklist

• I have read the Auth0 general contribution guidelines
• All existing and new tests complete without errors
• All active GitHub checks have passed

[cocojoe]

Can you add detailed, test steps please. Will have someone validate this. Thanks

[cocojoe]

@cysp please can you add a before / after video so I can see your testing in action. Thanks

[cocojoe]

Looks okay, follow up comments in main section.

[cysp]

Hi @cocojoe, sorry for the radio silence. 😬
I can certainly do up some recordings showing the behavioural difference! 🙂
This is an unreleased app, so would it be okay if I emailed them to you rather than posting them here in public? Is your git committer address okay for that?

[cocojoe]

Sure that's fine. 👍

[cysp]

Hi @cocojoe, I've finally gotten around to taking those screen recordings and have sent them through to the email address used in your commits in this repo.

[cocojoe]

Thanks, videos look fine. Can you rebase your branch please.

[cysp]

Cool, rebased. 🙂

[hzalaz]

Can we use a regular class/struct instead of an enum?

[cysp]

What kind of design do you have in mind using a class or struct?
Also is there a rationale for avoiding enumerations? They're a first-class type in Swift and this use-case seems like a perfect fit for an enumeration.

[hzalaz]

I dont see it as a natural use of an enum or us having more options than these two, maybe something like this:

struct NativeAuthSession {

  @available(iOS 12.0, *)
  private var authServiceSession: ASWebAuthenticationSession?
  private var safariSession: SFAuthenticationSession?
  
  @available(iOS 12.0, *)
  init(_ session: ASWebAuthenticationSession) {
    this.authServiceSession = session;
  }
 
  init(_ session: SFAuthenticationSession) {
    this.safariSession = session;
  }

  func start() -> Bool {
    if #available(iOS 12.0, *) {
      this.authServiceSession.start();
    } else {
      this.safariServices.start();
    }
  }

  func cancel() {
    if #available(iOS 12.0, *) {
      this.authServiceSession?.cancel();
    } else {
      this.safariServices?.cancel();
    }
  }
}

(might have typos since I coded it on the GH UI)

But probably is going into too mich details

[cysp]

The way I see it, we have the situation where we know that an object is one of two types, and that we will only have one, not both of those types of object. To me this seems like a perfect fit for an enum, since we can encode the fact that there's exactly one of those objects, and the type of that object into the type system.
The design where a struct has multiple optional properties feels like a less good fit because the type system admits the possibility that both of those properties may be nil (or not nil) at the same time, and requires unwrapping the optional in order to operate on the contained object.

Is there a style guide or a reason that you would prefer not to use an enum for this use case?

[cocojoe]

@cysp ☝️

[cocojoe]

@cysp can you re-base please and we can get this merged, I'm guessing there are a couple of conflicts now from most recent PR #286 as there is no option to update for me.

[cysp]

Sure, I've rebased onto master and it looks good to me.