-
Notifications
You must be signed in to change notification settings - Fork 419
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
@W-16362973: [iOS] Add QR Code Login Support in MSDK #3759
base: dev
Are you sure you want to change the base?
@W-16362973: [iOS] Add QR Code Login Support in MSDK #3759
Conversation
/** The biometric log in button */ | ||
@property (nonatomic, strong, readonly, nullable) UIButton *biometricButton; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this (going to be) used somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unknown, as of yet. Stay tuned for a more final and review-worthy commit soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's more detail coming in a few minutes when the template app pull request gets its final self-review commentary, but I would like to use this in the template app. Hopefully that'll look groovy when you get to that point.
…CodeLoginEnabled`. Functional P.O.C.)
65f19ee
to
38ba3b0
Compare
…ic Handling Of Query String Vs. Fragment)
…thHelper` With QR Code Log In Parameters)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #3759 +/- ##
==========================================
- Coverage 43.43% 43.32% -0.11%
==========================================
Files 223 223
Lines 20587 20637 +50
==========================================
Hits 8941 8941
- Misses 11646 11696 +50
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Here's the APEX for user agent flow:
|
Here's the APEX for web server flow. Note, @wmathurin and I only have one page and controller in place. We've been toggling the APEX content as we switch between tests. We could scale that so it is more convenient.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's my self-review notes and code walk-though.
@@ -225,6 +225,9 @@ NS_SWIFT_NAME(SalesforceManager) | |||
*/ | |||
@property (nonatomic, assign) BOOL isLoginWebviewInspectable; | |||
|
|||
/*** Indicates if login via QR Code and UI bridge API is enabled */ | |||
@property (nonatomic, assign) BOOL isQrCodeLoginEnabled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matches a new property also added on the Android side. It's use, however, is subtly different on iOS due to differences in the logic flow of finishing authorization after the allow page. I'll add a comment below on that.
|
||
import Foundation | ||
|
||
public extension SalesforceLoginViewController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Swift extension of the Objective-C SFLoginViewController
translates almost the exact same logic for QR Code Log In methods from Android Kotlin here to Swift. As is often the case, I was hoping to keep the logical implementation as similar as possible between the two platforms.
print("Login With Frontdoor Bridge URL: '\(frontdoorBridgeUrlString)'/'\(String(describing: pkceCodeVerifier))'.") | ||
|
||
guard let frontdoorBridgeUrl = URL(string: frontdoorBridgeUrlString) else { return } | ||
guard let webView = oauthView as? WKWebView else { return } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a safe use of oauthView
in MSDK iOS? Is there any more type-safe way to access the web view being used for log in? Note, QR Code Log In is web login specific.
@@ -791,7 +791,7 @@ - (void)webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigati | |||
NSURL *url = navigationAction.request.URL; | |||
NSString *requestUrl = [url absoluteString]; | |||
if ([self isRedirectURL:requestUrl]) { | |||
if ([[SalesforceSDKManager sharedManager] useWebServerAuthentication]) { | |||
if ([[SalesforceSDKManager sharedManager] useWebServerAuthentication] && ![[SalesforceSDKManager sharedManager] isQrCodeLoginEnabled]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As promised above, here's the only current use of isQrCodeLoginEnabled
on the iOS side. This is different than the Android logic which uses it for checking the handling of deep-linked intents from external QR code readers. Here, we need a way to send the received callback/redirect URL to handleUserAgentResponse
since that's the iOS logic to extracting the URL fragment with the authorization parameters.
The logic in handling the callback URL with the authorization parameters fragment after the allow and deny prompt is very different between Android and iOS. I was hesitant to tamper with useWebServerAuthentication
, but perhaps we could simply set that to false automatically when isQrCodeLoginEnabled
is true? That might work is the two are implicitly coupled. @wmathurin?
/** The biometric log in button */ | ||
@property (nonatomic, strong, readonly, nullable) UIButton *biometricButton; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's more detail coming in a few minutes when the template app pull request gets its final self-review commentary, but I would like to use this in the template app. Hopefully that'll look groovy when you get to that point.
request.codeVerifier = self.codeVerifier; | ||
|
||
// Choose either the default generated code verifier or the code verifier matching the overriding Salesforce Identity API UI Bridge front door bridge. | ||
request.codeVerifier = self.overrideWithCodeVerifier ? self.overrideWithCodeVerifier : self.codeVerifier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the selection between the existing code verifier that would be used for the default login URL or the code verifier passed from QR code login.
@@ -750,8 +756,8 @@ - (NSString *)approvalURLForEndpoint:(NSString *)authorizeEndpoint | |||
|
|||
if (!codeChallenge) { | |||
// Code verifier challenge: | |||
// - self.codeVerifier is a base64url-encoded random data string | |||
// - The code challenge sent here is an SHA-256 hash of self.codeVerifier, also base64url-encoded | |||
// - self.codeVerifier is a Base64 URL-Safe encoded (Note, not URL encoded) random data string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this comment since it is quite a significant difference. These values are specifically being Base64 URL/Filename Safe Encoded, which is actually what the iOS code already does. This caused some confusion when I was comparing it with the APEX code where many users are erroneously posting that the code challenge should be URL encoded when it actually must be Base 64 then Base 64 URL-safe encoded (which isn't the same). I hope this being more specific will be less misleading in the future.
[self handleWebServerResponse:url]; | ||
// Determine if presence of override parameters requiring the user agent flow. | ||
BOOL overrideWithUserAgentFlow = self.overrideWithfrontDoorBridgeUrl && !self.overrideWithCodeVerifier; | ||
if ( [[SalesforceSDKManager sharedManager] useWebServerAuthentication] && !overrideWithUserAgentFlow) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bbirman, @brandonpage and @wmathurin - This works, but I wonder if it's the best way to "override" to the flow needed by the QR code Vs. the app's preference. I tested both QR code types plus logging in as usual via the web view. It gives me the control flow I expected, but I would love more eyes on this spot.
If it helps to paraphrase the logic, the two "override" parameters are only present when calling through the auth helper with them from a QR code log in. A front door URL by itself needs user agent flow and with a code verifier requires web server flow.
Perhaps it's coincidental, but I noticed the user agent flow uses a fragment string #
and the web server flow uses a query string ?
. I tried it out, just switching on that, and it actually works perfectly since these two paths are hard coded to those URL formats. Would it be simpler to switch on the URL format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we should count on the URL format staying the same. I would rather having it based on something we truly control (like the presence or absence of the code verifier in the QR url).
Alternatively, we could have another parameter encoded in the QR url to indicate explicitly the flow to use: useUserAgentFlow = true | false.
Or like you said, we could just fall back on the the app configuration itself in which case a QR url without code verifier in an app configured to use web server flow, would end up using web server flow without pkce.
What am I saying? I think we can start with the approach you implemented above (driven by the QR url but without a dedicated param).
@@ -192,14 +192,24 @@ Set this block to handle presentation of the Authentication View Controller. | |||
|
|||
- (BOOL)authenticateUsingIDP:(SFSDKAuthRequest *)request completion:(SFUserAccountManagerSuccessCallbackBlock)completionBlock failure:(SFUserAccountManagerFailureCallbackBlock)failureBlock; | |||
|
|||
- (BOOL)authenticateWithRequest:(SFSDKAuthRequest *)request completion:(SFUserAccountManagerSuccessCallbackBlock)completionBlock failure:(SFUserAccountManagerFailureCallbackBlock)failureBlock; | |||
- (BOOL)authenticateWithRequest:(SFSDKAuthRequest *)request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These overloads will collapse significantly in Swift - someday.
@param codeVerifier Optionally and only with the front door bridge URL parameter, a code verifier to use | ||
when the front door bridge URL is using web server authentication | ||
*/ | ||
+ (void)loginIfRequired:(nullable UIScene *)scene |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the key public API for QR code login from the app.
🎸 Ready For Review! 🥁
This adds the MSDK logic needed to support QR Code Log In to MSDK iOS, much like its Android counterpart.
This includes support for both web server flow and user agent flow in the QR code. Most of the Quip written by @wmathurin still applies, but be aware the APEX code has changed significantly for web server flow. I'll post the updated APEX here as well.
I added a really detailed code walkthrough in my self-review. I highly recommend reading it in detail this time, since this is my personal first foray into this tenured section of the authentication logic. Do share thoughts on the pattern I used for connecting the new parameters to and then using them in the existing authentication logic.
Be sure to see the follow-up pull request, W-16171422: [iOS] Update Native Login Sample App to include QR Code Login Flow.
Thanks for reading, as always.