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

Adding support for specifying what browser needs to be used for authorization #655

Merged
merged 10 commits into from
Oct 15, 2022

Conversation

tdekoning
Copy link
Contributor

@tdekoning tdekoning commented Jul 21, 2021

This fixes #588 and #643

Description

This pull request adds the ability to control what browser is used for the authorization process. This helps in complex flows where the authorization is going through a browser and has a sidestep to a third party app.

In our case we were encountering issues, because third party apps use the default browser for return urls, but the in-app browser (Such as aswebauthenticationsession) doesn't share cookies with Safari.
By providing these parameters, the developers can fine-tune the login flow for their needs.

Question for the maintainers:
I'm not an iOS developer, and am seeing one flaw in the user flow when using an external browser:
If the user cancels the flow by pressing the "back" navigation on the top left of the browser, the promise isn't rejected. Do you have any idea how i can fix this? On Android, the promise is rejected when returning (which is expected)

Steps to verify

On iOS:

  • Adding iosCustomBrowser with the value safari should open safari, instead of a browser modal.
  • Without providing iosCustomBrowser, the behavior should remain the same as before this pull request

On Android:

  • Adding androidAllowCustomBrowsers with the value ['chrome'] should open chrome, instead of the in-app browser
  • Without providing androidAllowCustomBrowsers, the behavior should remain the same as before this pull request

@kadikraman
Copy link
Contributor

kadikraman commented Aug 15, 2021

Hi @tdekoning 👋 - thanks so much for taking a shot at this feature!

I just merged a PR to do with improving error messages on iOS which is unfortunately causing you some conflicts now. These should be pretty straightforward to solve - just merge main into your branch, choose your code from the diff and replace error localizedDescription with self getErrorMessage: error

Regarding user-cancelled-flow not working on iOS: I'm seeing this too, but I also don't get the success redirect. Can you confirm you've seen a successful auth flow using the Example app using Safari? Or was it using your own app?

@tdekoning
Copy link
Contributor Author

@kadikraman I've resolved the merge conflict

@martin-shaw
Copy link

martin-shaw commented Aug 23, 2021

This is similar to the feature I requested/proposed doing here #622

It would be nice if you could provide your own user agent to support other use cases, MDM managed browsers for example, rather than be limited to a fix set of browsers and have to make a code change to add support for a custom browser.

This would follow the intended usage of custom agents in AppAuth-iOS

@NicolaiOksen
Copy link

Any progress on this? It would be nice to be able to specify the desired browser for handling authentication.

@kadikraman
Copy link
Contributor

Apologies, I've been away from this project. Unfortunately there's merge conflicts again, I'm sorry! @tdekoning are you still around to resolve them?

@joelfsreis
Copy link

It would be amazing to have this feature available! Any news @tdekoning?

@tdekoning
Copy link
Contributor Author

@kadikraman I'm sorry for the delay, i had to focus on some other things.
Is this pull request a feature that is still desired for the library?

@kadikraman
Copy link
Contributor

Hi @tdekoning - I understand completely! We'd still love to have this feature. If you do find time to resolve the conflicts, please @ me in the PR and I'll review it immediately.

@android-imdad
Copy link

@tdekoning Yes please, I would very much like to have this feature

@tdekoning
Copy link
Contributor Author

@kadikraman I've fixed the merge conflicts.
However, i wasn't able to test it properly, since i no longer have a server set up with which i can authenticate. The example in this repository also seems to not fully work.
I did test opening the correct browsers when using the authorize function within the example-app and that opened the correct browser.

It would be awesome if you could do some quality checks to see if this fix actually works in a real life scenario.

@kadikraman
Copy link
Contributor

Thank you @tdekoning!

I see what you mean regarding: the example app. Looks like https://demo.identityserver.io/ is down (hopefully not permanently?) and the Auth0 example seems to have also changed since we configured it so it no longer redirects back like it used to. Fun. I'll fix the configuration first.

We'll test this on real device to be sure. Based on the code, I'm expecting that this will open the custom browser, but if the user doesn't have it installed, it will fall back to the default.

@android-imdad
Copy link

Hi @kadikraman Any update on merging this ? It would be great to have this soon.

@kadikraman
Copy link
Contributor

Looks like the identity server demo was moved so I updated the example app to point to the new instance #780

Merging this now 🚀

@kadikraman kadikraman merged commit f8c9162 into FormidableLabs:main Oct 15, 2022
@kadikraman
Copy link
Contributor

Just a head up I'm not able to publish this yet - looks like the latest main doesn't work for me on Android (getting invalid token on the identity server demo). I don't believe it's this change, but I don't want to do a release until we've confirmed it's not caused by one of the recently merged PRs.

@kadikraman
Copy link
Contributor

This is published in the next release candidate 7.0.0-rc1 🎉

@corne-de-bruin
Copy link

@kadikraman I've been debugging this InvalidTokenIssue as well since we really need this custom browser option in our project to support SSO.

I think this bug is introduced by upgrading the native AppAuth library to 1.6.
If I replace the following types inside the RNAppAuth.m file the example app works again.

OIDAuthorizationCallback -> OIDAuthStateAuthorizationCallback
OIDAuthorizationResponse -> OIDAuthorizationResponse
authorizationResponse -> authState
OIDAuthorizationService presentAuthorizationRequest:request -> _currentSession = [OIDAuthState authStateByPresentingAuthorizationRequest:request

RNAppAuth.m line: 375

OIDAuthStateAuthorizationCallback callback = ^(OIDAuthState *_Nullable authState, NSError *_Nullable error) {
                                                   typeof(self) strongSelf = weakSelf;
                                                   strongSelf->_currentSession = nil;
                                                   [UIApplication.sharedApplication endBackgroundTask:rnAppAuthTaskId];
                                                   rnAppAuthTaskId = UIBackgroundTaskInvalid;
                                                   if (authState) {
                                                       resolve([self formatResponse:authState.lastTokenResponse
                                                           withAuthResponse:authState.lastAuthorizationResponse]);
                                                   } else {
                                                       reject([self getErrorCode: error defaultCode:@"authentication_failed"],
                                                              [self getErrorMessage: error], error);
                                                   }
                                               };

    if (skipCodeExchange) {

        if(externalUserAgent != nil) {
            _currentSession = [OIDAuthState authStateByPresentingAuthorizationRequest:request
                                                                 externalUserAgent:externalUserAgent
                                                                          callback:callback];
        } else {
            if (@available(iOS 13, *)) {
                _currentSession = [OIDAuthState authStateByPresentingAuthorizationRequest:request
                                                          presentingViewController:presentingViewController
                                                           prefersEphemeralSession:prefersEphemeralSession
                                                                          callback:callback];
            } else {
                _currentSession = [OIDAuthState authStateByPresentingAuthorizationRequest:request
                                                          presentingViewController:presentingViewController
                                                                          callback:callback];
            }
        }
    } else {

        if(externalUserAgent != nil) {
            _currentSession = [OIDAuthState authStateByPresentingAuthorizationRequest:request
                                                                    externalUserAgent:externalUserAgent
                                                                             callback:callback];
        } else {
            OIDAuthStateAuthorizationCallback callback = ^(OIDAuthState *_Nullable authState,
                                                                NSError *_Nullable error) {
                                                        typeof(self) strongSelf = weakSelf;
                                                        strongSelf->_currentSession = nil;
                                                        [UIApplication.sharedApplication endBackgroundTask:rnAppAuthTaskId];
                                                        rnAppAuthTaskId = UIBackgroundTaskInvalid;
                                                        if (authState) {
                                                            resolve([self formatResponse:authState.lastTokenResponse
                                                                withAuthResponse:authState.lastAuthorizationResponse]);
                                                        } else {
                                                            reject([self getErrorCode: error defaultCode:@"authentication_failed"],
                                                                   [self getErrorMessage: error], error);
                                                        }
                                                    };
            if (@available(iOS 13, *)) {
                _currentSession = [OIDAuthState authStateByPresentingAuthorizationRequest:request
                                                                 presentingViewController:presentingViewController
                                                                  prefersEphemeralSession:prefersEphemeralSession
                                                                                 callback:callback];
            } else {
                _currentSession = [OIDAuthState authStateByPresentingAuthorizationRequest:request
                                                                 presentingViewController:presentingViewController
                                                                                 callback:callback];
            }
        }
    }

I'll didn't had time yet to look into Android yet.

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

Successfully merging this pull request may close these issues.

The ability to specify browser in authorize method
8 participants