Skip to content
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

Add option to prefer Safari View Controller #325

Closed
wants to merge 1 commit into from
Closed

Add option to prefer Safari View Controller #325

wants to merge 1 commit into from

Conversation

denisw
Copy link

@denisw denisw commented Oct 26, 2018

What

This PR adds a static setPreferSafariViewController: method to OIDExternalUserAgentIOS. If set to YES, the Safari View Controller is used even iOS 11 and 12 where SFAuthenticationSession and ASWebAuthenticationSession are normally used.

This should fix #209.

Why

We need to implement identity provider logout in an app. This requires us to open a special logout URL in a browser that shared the session (cookies) with the login browser's session. This is trivially possible with Safari View Controller, but not SFAuthenticationSession (see #209).

This is useful if you need the customizaition provided by
setSafariViewControllerFactory:, or if you need the authorization
browser session to be shared with other Safari View Controllers
in the app (e.g., for logout).
@codecov-io
Copy link

codecov-io commented Oct 26, 2018

Codecov Report

Merging #325 into master will decrease coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #325      +/-   ##
==========================================
- Coverage   74.05%   73.93%   -0.12%     
==========================================
  Files          58       58              
  Lines        5076     5084       +8     
==========================================
  Hits         3759     3759              
- Misses       1317     1325       +8
Impacted Files Coverage Δ
Source/iOS/OIDExternalUserAgentIOS.m 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbeffa6...0baf1f7. Read the comment docs.

@WilliamDenniss
Copy link
Member

WilliamDenniss commented Oct 26, 2018

You can do this already in AppAuth, see here: #209 (comment) The class in that snippit could be added to AppAuth, but I'm not sure it's needed, as I'm unconvinced that this is a mainstream use-case (but am still open minded if you have some good arguments).

Why can't you use logout with ASWebAuthenticationSession? I would think it should work.

Have you tried the dev-logout branch by any chance?

@denisw
Copy link
Author

denisw commented Oct 27, 2018

No, I didn't try the dev-logout branch yet - if that provides logout on all of iOS 9 - 12, then that would solve my logout problems! Thanks for the pointer.

I know that logout would work with ASWebAuthenticationSession (shared cookie jar with Safari), but not with SFAuthenticationSession (shares cookie jar only with itself in other apps). So iOS 11 is still a problem, unless I force the Safari View Controller on that version (which this PR allows).

There is another argument for adding this flag to the core codebase - the setSafariViewControllerFactory: API. If a developer relies on the customisations made using this method, it's kind of surprising if they just get ignored on newer iOS releases (because Safari View Controller is not used). Being able to force the Safari View Controller would be good in this case.

@denisw
Copy link
Author

denisw commented Oct 29, 2018

So I didn't have the chance to test the dev-logout branch, but from the code it seems to me that it uses the same external user agent for logout as it does for login. Doesn't that mean that in case of SFAuthenticationSession and ASWebAuthenticationSession, it would still show the "[App] wants to log in with [IdP]" message although you are logging out? If that's the case, I and others would probably still want the option to go all-SafariViewController easily for the time being.

@WilliamDenniss
Copy link
Member

WilliamDenniss commented Oct 29, 2018

from the code it seems to me that it uses the same external user agent for logout as it does for login

Yes, logout uses the same external user-agent by default as login.

the setSafariViewControllerFactory: API. If a developer relies on the customisations made using this method, it's kind of surprising if they just get ignored on newer iOS releases (because Safari View Controller is not used)

Good point that this API is confusing now. I think we should probably just remove it. Anyone who needs customizations can still achieve this by providing their own OIDExternalUserAgent implementation

There is another argument for adding this flag to the core codebase

The flag isn't needed. AppAuth supports pluggable external user-agents, you can drop your preferred one right in (including, for logout). I shared an example SFSafariViewController one that I created. If AppAuth was to include built-in SFSafariViewController support, we could just ship that class directly. For now we don't, but I'll add your use-case to the tally, we might include it if there's enough demand & valid use-cases.

Rather than overloading the default user agent OIDExternalUserAgentIOS with a bunch of different modes (like SFSVC only), I think the better approach is to provide multiple implementations of OIDExternalUserAgent. For example, we ship with one that will use Firefox or Chrome, instead of Safari.

still want the option to go all-SafariViewController easily for the time being

Fair enough.

Did you try the code I shared? That will allow you to do this without forking.

@WilliamDenniss
Copy link
Member

The setSafariViewControllerFactory API has been removed from the 1.0 branch to reduce confusion (it's only relevant for iOS 10 and below, which is increasingly not very relevant). The gist was updated to add this API there, for those who want the SFSafariViewController user agent only.

@WilliamDenniss
Copy link
Member

Closing this PR, as the functionality is possible without these changes needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support sign on without SSO (no SFAuthenticationSession)
3 participants