Added ASWebAuthenticationSession Support iOS12 #245

cocojoe · 2018-11-15T14:50:48Z

Changes

Follow up to #239

Internal changes to enable ASWebAuthenticationSession in iOS 12 and maintain support for Xcode 9.

References

#238

Testing

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

Checklist

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

Added Lib to Podfile Reverted Scanfile

cocojoe · 2018-11-15T15:20:46Z

@martinoconnor Was a little tricker to add backwards compatibility for Xcode 9. However, all good and passing all platform tests.

cocojoe · 2018-11-20T11:28:07Z

Auth0/SafariAuthenticationSession.swift

        super.init(redirectURL: redirectURL, state: state, handler: handler, finish: finish, logger: logger)
-        self.authSession = SFAuthenticationSession(url: self.authorizeURL, callbackURLScheme: self.redirectURL.absoluteString) { [unowned self] in
+        #if canImport(AuthenticationServices)


So turns out there was this handy pre-processor added into Swift 4.1 that let's us check for a Framework.

cocojoe · 2018-11-20T11:28:16Z

Auth0/SafariAuthenticationSession.swift

+            self.authSession = authSession
+            authSession.start()
+        }
+        #else


This will be fallback in Xcode 9

cocojoe · 2018-11-20T11:29:34Z

Auth0/SafariAuthenticationCallback.swift

    init(url: URL, schemeURL: String, callback: @escaping (Bool) -> Void) {
        self.callback = callback
-        self.authSession = SFAuthenticationSession(url: url, callbackURLScheme: schemeURL) { [unowned self] url, _ in
+        #if canImport(AuthenticationServices)


As this Framework does not exist in Xcode 9, it can not compile without this.

I'd swap the conditions to return earlier in the code:

#if NOT canImport(AuthenticationServices){ ... authSession.start() return. } // No need for "else" here if #available(iOS 12.0, *) { ... } else { ... } self.authSession.start()

Also, consider adding this same comment on a line above the if check, whenever applicable, as would help whoever maintains this in the future to understand it.

These are two different kinds of checks, the #if is a pre-processor so if it's on Xcode 10+ include this code. If it was Xcode 9 only the code in the #else would actually be visible to the compiler. The if #available(iOS12) is a runtime check, no escaping it.

cocojoe · 2018-11-20T11:29:37Z

Auth0/SafariAuthenticationCallback.swift

@@ -22,22 +22,45 @@

 import UIKit
 import SafariServices
+#if canImport(AuthenticationServices)


As this Framework does not exist in Xcode 9, it can not compile without this.

cocojoe · 2018-11-20T11:29:54Z

Auth0/SafariAuthenticationCallback.swift

+            self.authSession = authSession
+            authSession.start()
+        }
+        #else


Fallback Xcode 9

cocojoe · 2018-11-20T11:30:14Z

Auth0/SafariAuthenticationSession.swift

@@ -22,20 +22,50 @@

 import UIKit
 import SafariServices
+#if canImport(AuthenticationServices)


As this Framework does not exist in Xcode 9, it can not compile without this.

cocojoe · 2018-11-20T11:30:28Z

Auth0/SafariAuthenticationSession.swift

+                }
+            self.authSession = authSession
+            authSession.start()
+        } else {


Fallback iOS 11.0 (This whole class is only available in iOS 11.0+)

cocojoe · 2018-11-20T11:30:57Z

Auth0/SafariAuthenticationCallback.swift

+            }
+            self.authSession = authSession
+            authSession.start()
+        } else {


Fallback iOS 11.0 (This whole class is only available in iOS 11.0+)

lbalmaceda · 2018-11-22T17:14:24Z

Auth0/SafariAuthenticationCallback.swift

    init(url: URL, schemeURL: String, callback: @escaping (Bool) -> Void) {
        self.callback = callback
-        self.authSession = SFAuthenticationSession(url: url, callbackURLScheme: schemeURL) { [unowned self] url, _ in
+        #if canImport(AuthenticationServices)


I'd swap the conditions to return earlier in the code:

#if NOT canImport(AuthenticationServices){ ... authSession.start() return. } // No need for "else" here if #available(iOS 12.0, *) { ... } else { ... } self.authSession.start()

Also, consider adding this same comment on a line above the if check, whenever applicable, as would help whoever maintains this in the future to understand it.

lbalmaceda · 2018-11-22T17:16:08Z

Auth0/SafariAuthenticationCallback.swift

            self.callback(url != nil)
            TransactionStore.shared.clear()
        }
-        self.authSession?.start()
+        self.authSession = authSession


why are you doing the assignment here and not above? e.g.

self.authSession = SFAuthenticationSession(url: url, callbackURLScheme: schemeURL) .....

I see this in many files of this PR

After re-reading this.. Note that "fallback for 11" and "fallback for 9" have the exact same code. The only thing changing is SFAuthenticationSession and ASWebAuthenticationSession on 1 of the 3 cases. Please consider re-writing the code so that only that first line is wrapped around the OS check and the remaining is shared.

I imagine something like this is possible:

self.callback = callback #if canImport(AuthenticationServices) && #available(iOS 12.0, *) self.authSession = ASWebAuthenticationSession(url: url, callbackURLScheme: schemeURL) { [unowned self] url, _ in self.callback(url != nil) TransactionStore.shared.clear() } #else self.authSession = SFAuthenticationSession(url: url, callbackURLScheme: schemeURL) { [unowned self] url, _ in self.callback(url != nil) TransactionStore.shared.clear() } #endif self.authSession.start()

The behaviour is slightly more complex than this, I could assign self.authSession = ASWeb/SFAuth However at that point it is now a common foundation type of NSObject. This is done so we can reuse one property, it's only real purpose here is to retain the object so the AuthenticationSession doesn't vanish until it's completed.

Problem now is you can't call self.authSession.start() as start() is not available in NSObject.

what about then,

self.callback = callback let authSession; #if canImport(AuthenticationServices) && #available(iOS 12.0, *) authSession = ASWebAuthenticationSession(url: url, callbackURLScheme: schemeURL) { [unowned self] url, _ in self.callback(url != nil) TransactionStore.shared.clear() } #else authSession = SFAuthenticationSession(url: url, callbackURLScheme: schemeURL) { [unowned self] url, _ in self.callback(url != nil) TransactionStore.shared.clear() } #endif self.authSession = authSession; authSession.start()

It's a good idea in theory. However, you're left with the same problem, Swift is strongly type, you must specify the type for your let authSession So you are back to the same problem as your previous suggestion.

lbalmaceda

Left some suggestions but can't be applied because swift is a strongly typed language. The changes look OK anyway, so ⚡️

Martin O'Connor and others added 3 commits November 15, 2018 13:43

Use ASWebAuthenticationSession on iOS 12

c4f11c6

Update test configuration

3b6fe22

Fixed compiling in Xcode 9

874f71c

Added Lib to Podfile Reverted Scanfile

cocojoe mentioned this pull request Nov 15, 2018

Use ASWebAuthenticationSession on iOS 12 #239

Closed

cocojoe changed the title ~~Added ASWebAuthenticationSession Support~~ Added ASWebAuthenticationSession Support iOS12 Nov 15, 2018

Merge branch 'master' into added-aswebauthentication

b6ce085

cocojoe commented Nov 20, 2018

View reviewed changes

Auth0/SafariAuthenticationCallback.swift

self.authSession = authSession

authSession.start()

}

#else

Copy link

Member Author

cocojoe Nov 20, 2018

Choose a reason for hiding this comment

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

Fallback Xcode 9

cocojoe commented Nov 20, 2018

View reviewed changes

lbalmaceda reviewed Nov 22, 2018

View reviewed changes

lbalmaceda approved these changes Nov 22, 2018

View reviewed changes

cocojoe added the CH: Added label Nov 22, 2018

cocojoe added this to the vNext milestone Nov 22, 2018

cocojoe merged commit 294c63a into master Nov 22, 2018

cocojoe mentioned this pull request Dec 6, 2018

Release 1.14.0 #246

Merged

cocojoe deleted the added-aswebauthentication branch July 18, 2019 09:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ASWebAuthenticationSession Support iOS12 #245

Added ASWebAuthenticationSession Support iOS12 #245

cocojoe commented Nov 15, 2018 •

edited

Loading

cocojoe commented Nov 15, 2018

cocojoe Nov 20, 2018

cocojoe Nov 20, 2018

cocojoe Nov 20, 2018 •

edited

Loading

lbalmaceda Nov 22, 2018

cocojoe Nov 22, 2018

cocojoe Nov 20, 2018

cocojoe Nov 20, 2018

cocojoe Nov 20, 2018

cocojoe Nov 20, 2018 •

edited

Loading

cocojoe Nov 20, 2018 •

edited

Loading

lbalmaceda Nov 22, 2018

lbalmaceda Nov 22, 2018

lbalmaceda Nov 22, 2018

cocojoe Nov 22, 2018

lbalmaceda Nov 22, 2018

cocojoe Nov 22, 2018

lbalmaceda left a comment

Added ASWebAuthenticationSession Support iOS12 #245

Added ASWebAuthenticationSession Support iOS12 #245

Conversation

cocojoe commented Nov 15, 2018 • edited Loading

Changes

References

Testing

Checklist

cocojoe commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocojoe Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocojoe Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

cocojoe Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda left a comment

Choose a reason for hiding this comment

cocojoe commented Nov 15, 2018 •

edited

Loading

cocojoe Nov 20, 2018 •

edited

Loading

cocojoe Nov 20, 2018 •

edited

Loading

cocojoe Nov 20, 2018 •

edited

Loading