Skip to content
This repository has been archived by the owner on Jan 21, 2022. It is now read-only.

Add option for user to select browser type to use for browser based authentication #265

Open
wants to merge 29 commits into
base: develop
Choose a base branch
from

Conversation

sandersaelmans
Copy link

@sandersaelmans sandersaelmans commented Dec 8, 2020

Issue

Currently a SFSafariViewController is used for browser based authentication. This is fine as long as cookie sharing is not needed between multiple browser instances; However some social logins, especially ones using the app to authenticate the user, require these cookies to be shared. As of iOS 11, SFSafariViewController does not share cookies with the Safari app, nor any other browser instance. As of iOS 11 Apple introduced dedicated authentication sessions to handle authenticating a user through a social login: SFAuthenticationSession(deprecated), and shortly after ASWebAuthenticationSession.

Changes

This PR adds the option for users to select which type of browser based authentication type they want to use SFSafariViewController or ASWebAuthenticationSession. The latter is only available if the user is running on a target which supports it (>= iOS 12, >= OSX 10.15).

This can be done by setting it on the top level MAS object:

// objc
[MAS preferredBrowserBasedAuthenticationType:MASBrowserBasedAuthenticationTypeSafari];

// swift
MAS.preferredBrowserBasedAuthenticationType(.safari)

Upon starting browser based authentication, the preferred browser type will be used, instead of the default SFSafariViewController.

Future Changes

Currently there is a gap for iOS 11 users, as ASWebAuthenticationSession is still available from iOS 12 and onwards. We could opt to also support the deprecated SFAuthenticationSession if needed.

BobbyWeber and others added 27 commits December 14, 2017 12:12
* Stable:
  Update CHANGELOG.md
* Stable: (23 commits)
  Update CHANGELOG.md
  MASFoundation : removing the library that is no longer being used
  MASFoundation : version update for 1.6.10 release
  Copy edits for clarity
  US461954 : [iOS][MASFoundation] Refactor MASRequest Invoke's Response Structure
  Removed set parameters methods
  Copy edits
  US461954 : [iOS][MASFoundation] Refactor MASRequest Invoke's Response Structure
  [iOS][MASFoundation] ChangeLog update
  Deprecated additional methods.
  Fix for issue described in DE346442
  [DE343807] App crash fixed
  [DE343807] Fixed app crash with startWithURL
  US436059 : [iOS] Allow MQTT connections to servers with public cert - update for code review’s change requests
  DE339566 : [iOS][MASFoundation] - issue sending special character like "&" or "+" for login password
  US436059 : [iOS][MASFoundation] MQTT to fix some delegation, and block handler
  DE331046 : [iOS][MASFoundation] : Device name in DN
  DE331046 : [MASFoundation][iOS] Device name for DN
  US436059 :  [iOS] Allow MQTT connections to servers with public cert - code cleanup
  US436059 : [iOS] Allow MQTT connections to servers with public cert - adding reconnection management
  ...
# Conflicts:
#	CHANGELOG.md
@zakirjt zakirjt requested a review from ysd24 December 16, 2020 14:48
Copy link
Contributor

@ysd24 ysd24 left a comment

Choose a reason for hiding this comment

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

We came to this method "handleAuthorizationResponseURL" through a custom scheme. We might need to evaluate if 'callbackURLScheme' could be used too.

{
self.session = [[ASWebAuthenticationSession alloc] initWithURL:url callbackURLScheme:nil completionHandler:^(NSURL * _Nullable callbackURL, NSError * _Nullable error) {
if (callbackURL != nil) {
[MASAuthorizationResponse.sharedInstance handleAuthorizationResponseURL:callbackURL];
Copy link
Contributor

Choose a reason for hiding this comment

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

We came to this method through a custom scheme. We might need to evaluate if 'callbackURLScheme' could be used too.

Copy link
Author

@sandersaelmans sandersaelmans Dec 17, 2020

Choose a reason for hiding this comment

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

Good point. nil only works if it's the only url scheme a customer has specified in their URL types. I'll add a way for a customer to supply this parameter as well.

@ysd24: Do you have a preferred way to supply this? I can either:

  1. provide it via MAS -> MASModelService
  2. rewrite the the enum to some sort of configuration object which the user can provide.

Copy link
Author

Choose a reason for hiding this comment

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

@ysd24 updated PR so users an provide the call back url scheme when using a web session

DLog(@"Browser cancel clicked");
}
});
MASBrowserBasedAuthenticationType type = [MASModelService browserBasedAuthenticationType];
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to think about backward compatibility, in terms of an existing customer who uses the 'BrowserBasedLogin'

Copy link
Author

Choose a reason for hiding this comment

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

If a customer doesn't set a preferred browser type the old implementation (SFSafariViewController) will be used. If the implementation is correct existing customers should not be impacted when updating to a newer version of the SDK.

@ysd24
Copy link
Contributor

ysd24 commented Dec 16, 2020

We have to think about backward compatibility, in terms of an existing customer who uses the 'BrowserBasedLogin' should not need to set the flag, instead the default behaviour has to be intact.

Copy link
Contributor

@ysd24 ysd24 left a comment

Choose a reason for hiding this comment

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

** Looks all good to me.
But had a bit of a hitch with taking an input for a custom url scheme from the customer.
I am coming from the perspective of 'its a SDK/Library', we might don't want to ask for the custom url scheme of the app.
What i am trying to see here is, we open the web login template on the ASWebAuthSession and upon the completion it redirects to the custom url scheme, this kicks the app re-opened again and the app delegate, delegates the callback url to the AuthResponse class. Is it possible through ASWebSession ??

* @param callbackURLScheme NSString containing the callback url scheme to use.
* @return Returns the newly initialized MASWebSessionBrowserBasedAuthentication.
*/
- (instancetype)initWithCallbackURLScheme:(NSString *)callbackURLScheme;
Copy link
Contributor

Choose a reason for hiding this comment

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

** Looks all good to me.
But had a bit of a hitch with taking an input for a custom url scheme from the customer.
I am coming from the perspective of 'its a SDK/Library', we might don't want to ask for the custom url scheme of the app.
What i am trying to see here is, we open the web login template on the ASWebAuthSession and upon the completion it redirects to the custom url scheme, this kicks the app re-opened again and the app delegate, delegates the callback url to the AuthResponse class. Is it possible through ASWebSession ??

Copy link
Author

Choose a reason for hiding this comment

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

I removed the need to provide the url scheme altogether for the WebSession browser authentication type. This will be extracted from the MSSO config.

…rowser type. Also improved interface when setting the type of browser used for swift.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants