Implement support for SFSafariController on iOS

[john-slow]

This fixes this issue:

538

Implemented support for SFSafariController on iOS so that user can choose between ASWebAuthenticationSession, EphemeralSession and SFSafariViewController.

Reason: ASWebAuthenticationSession is having a displaying bug on iOS which would show "sign in" instead of "sign out" when user logs out. However, EphemeralSession is not fulfilling our requirement because it is using a private browser which doesn't save the cache. So user has to input the password every time when they login. SFSafariViewController would be the better choice between these three because it doesn't show the alert and keep the cache at the same time.

Additional: To implement these changes, I had to modify the flutter_appauth_platform_interface. The changes are in the first commit. They would need to be published first on pub.dev. In the meantime, I access the change directly by referencing the path instead of the version.

[vmichalak]

Thanks a lot for this PR. It's gonna helps me a lot :)

[mchan-genetec]

+1 to add this functionality to the repo

[john-slow]

Hey @MaikuB, is it possible that you could review my pr, many thanks!

[MaikuB]

@john-slow Hi I'm aware of your PR and will do a proper review when I can as the work I do with open source projects is done so on spare time. In the mean time, you and others should be to reference your fork in your app as Flutter/Dart projects can references dependences via Git.

At a glance, one I did spot is if you could revert the pubspec.yaml changes that you presumably did for development purposes.

One thing I do wonder though if could risk causing apps to be rejected. Whilst the messaging shown using ASWebAuthenticationSession when signing out is wrong, it is the API more geared for auth flows. My understanding is it's also meant to be able to share browser state with Safari as well whilst SFSafariController doesn't and the API docs for it even mention to use ASWebAuthenticationSession instead. Though the main concern around this that I have is that how this goes against best practices that I'm not sure if the app store will check as part of app submission. Have you tried to submit an app using the changes to see what Apple came back with?

[john-slow]

@john-slow Hi I'm aware of your PR and will do a proper review when I can as the work I do with open source projects is done so on spare time. In the mean time, you and others should be to reference your fork in your app as Flutter/Dart projects can references dependences via Git.

At a glance, one I did spot is if you could revert the pubspec.yaml changes that you presumably did for development purposes.

One thing I do wonder though if could risk causing apps to be rejected. Whilst the messaging shown using ASWebAuthenticationSession when signing out is wrong, it is the API more geared for auth flows. My understanding is it's also meant to be able to share browser state with Safari as well whilst SFSafariController doesn't and the API docs for it even mention to use ASWebAuthenticationSession instead. Though the main concern around this that I have is that how this goes against best practices that I'm not sure if the app store will check as part of app submission. Have you tried to submit an app using the changes to see what Apple came back with?

Hi @MaikuB, thanks for your prompt response and suggestions! My team has already submitted and published an app that utilizes SFSafariController, and Apple has approved it; it’s been on the App Store for over 8 months now. Additionally, since Apple's API allows for a custom externalAgent, I believe this indicates their acceptance of it.

Regarding the changes to pubspec.yaml, would you like me to create two separate PRs—one for appauth and another for appauth_platform_interface—so that you can update the version of appauth_platform_interface in the appauth pubspec.yaml?

[MaikuB]

Regarding the changes to pubspec.yaml, would you like me to create two separate PRs—one for appauth and another for appauth_platform_interface—so that you can update the version of appauth_platform_interface in the appauth pubspec.yaml?

Same PR is fine and you don't need to make changes to both pubspec files. I'll do that myself when it comes time to publishing and the changes at that point are version bumps.

I might be wrong but have a suspicion you weren't aware that the plugin is setup to use melos. If so, read https://github.com/MaikuB/flutter_appauth/blob/master/CONTRIBUTING.md#environment-setup.

Another thing that's useful to note for future reference that you may also want to do now is that it's better to create a same branch for changes you intend to make. In other words, it's better that this PR came from a different branch. This way, your fork's master branch is consistent with the master branch of the repository you fork from. This helps avoid running into difficulties if the original repo pushed changes to master

Edit: one thing I forgot to say, is thanks a lot for submitting the PR!

[john-slow]

Same PR is fine and you don't need to make changes to both pubspec files. I'll do that myself when it comes time to publishing and the changes at that point are version bumps.

I might be wrong but have a suspicion you weren't aware that the plugin is setup to use melos. If so, read https://github.com/MaikuB/flutter_appauth/blob/master/CONTRIBUTING.md#environment-setup.

Another thing that's useful to note for future reference that you may also want to do now is that it's better to create a same branch for changes you intend to make. In other words, it's better that this PR came from a different branch. This way, your fork's master branch is consistent with the master branch of the repository you fork from. This helps avoid running into difficulties if the original repo pushed changes to master

Edit: one thing I forgot to say, is thanks a lot for submitting the PR!

You're welcome! I'm happy to help. I reverted my changes to the pubspec.yaml file, and I appreciate your advice—it will definitely be useful for future reference. However, I can't change the source branch for this PR, so I'll continue with the master branch instead of creating a new one. Otherwise, I'd have to abandon this PR and start over, which feels a bit messy. Also, since there's no direct equivalent of SFSafariController in macOS, I used the default OIDExternalUserAgentMac even user uses SFSafariViewController parameter. You might want to note that in your version bump as well.

[MaikuB]

Thanks again for this. You may see me do some minor cleanup/changes that I figured would be easier if I do to avoid more back and forth :)

Not related to the PR but should you do more PRs, I'd suggest avoiding the use of force push as well. A couple of reasons why I suggest this is force pushes makes its harder to see the differences and it results in conflicts when someone has cloned your fork/branch before and needs to get the latest changes

[MaikuB]

There's now a 8.0.0 prerelease with these changes. I had renamed the enum, docs and related properties so that it (IMO) aligns more with what is done by the plugin and/or the AppAuth SDK

[john-slow]

Thanks again for this. You may see me do some minor cleanup/changes that I figured would be easier if I do to avoid more back and forth :)

Not related to the PR but should you do more PRs, I'd suggest avoiding the use of force push as well. A couple of reasons why I suggest this is force pushes makes its to see the differences and it results in conflicts when someone has cloned your fork/branch before and needs to get the latest changes

You're very welcome! Thank you for your advice and for accepting my pull request. I'm excited for the 8.0.0 release!

Edit: BTW, in the prerelease 8.0.0, you returns both OIDExternalUserAgentIOSNoSSO instead of OIDExternalUserAgentIOSSafariViewController on sfSafariController enum type.

[vmichalak]

Thanks again for this. You may see me do some minor cleanup/changes that I figured would be easier if I do to avoid more back and forth :)

Not related to the PR but should you do more PRs, I'd suggest avoiding the use of force push as well. A couple of reasons why I suggest this is force pushes makes its harder to see the differences and it results in conflicts when someone has cloned your fork/branch before and needs to get the latest changes

I was using the fork while doing some tests, I had a little surprise when I wanted to rebuild my test 🤣

[mkhtradm01]

Thanks a lot for this PR. It really saves my hustle, it fixes our bottleneck