okta / okta-oidc-android

OIDC SDK for Android
https://github.com/okta/okta-oidc-android
Other
59 stars 45 forks source link

Config change during signin/signout #236

Closed coreform closed 2 years ago

coreform commented 3 years ago

Given this lib's API requires integrators to pass an Activity in to build a WebAuthClient and to signin and signout, it is no wonder that this lib isn't sociable with configuration changes. Given the need to allow configuration changes that would destroy and rebuild an Activity, do you have any guidance as to how to deal with that?

I take it the lib does some ActivityResult handling under the hood and of course the Activity is needed for that. The old appauth lib had the integrator do that work (and perhaps was more flexible in that regard), while the new one seems to force use and binding to a single Activity by doing it itself. An orientation change while the Chrome Custom Tab is open and displaying the Okta flow will see the Activity it was started from destroyed and the ResultCallback orphaned, never to be called back and any ActivityResult expectations the Okta lib had will not eventuate. To ride-out configuration changes, however, a better host for the callback and that mechanism would be within a non-configuration instance Fragment if possible.

NikitaAvraimov-okta commented 3 years ago

@coreform thanks for the question. We`ll discuss it with the team to share recommended approach

fn-jt commented 3 years ago

@NikitaAvraimov-okta any advice? Our app is getting backgrounded and destroyed when navigating to Chrome, causing our onActivityResult to never fire.

FeiChen-okta commented 3 years ago

Hi @fn-jt I can't reproduce this issue with the SampleActivity. The startActivityForResult does survive orientation changes. In the origin Activity are you overriding onActivityResult?

wyland commented 3 years ago

Hi @FeiChen-okta , I'm on the same team as @fn-jt and can answer your question.

Thats correct, we are overriding the onActivityResult in our Activity with the following code:

override fun onActivityResult(requestCode: Int, resultCode: Int, data: Intent?) {
        super.onActivityResult(requestCode, resultCode, data)
        //must pass the results back to the WebAuthClient.
        val access_token = oktaLogin?.handleActivityResult(requestCode, resultCode, data)
        if(access_token.isNullOrEmpty()){
          AlertDialog.Builder(this)
          // show error message
        } else {
          // complete login
        }
    }

When adding a breakpoint here, we never seem to reach this since this Activity is getting destroyed and recreated. However if we immediately try to login again and open the chrome window, the callback succeeds on the second time and we hit our breakpoint. This error only happens on the first try, please fully delete all Data/Cache in your chrome app before testing. Thanks

FeiChen-okta commented 3 years ago

Hi @wyland Try putting registerCallback in onCreate or call it again in onConfigurationChange. The SDK unregister the callback if the activity is destroyed.

fn-jt commented 3 years ago

Hey @FeiChen-okta, I tried it in onCreate, and it didn't work. I think the real problem is that RedirectActivity doesn't even get hit. I put a breakpoint in RedirectActivity.onCreate(), and it didn't trigger the first time I log in (though it does the second time). Also, wouldn't onActivityResult() get triggered before the callback?

FeiChen-okta commented 3 years ago

Hi @fn-jt RedirectActivity is where it receives the login redirect from Chrome. Device rotation does nothing to this since it is not started during the login flow. Once the user have signed in, then android should start RedirectActivity with the schema. All it does is bounce the result back to OktaAuthenticationActivity. This is where it send the results back to your application with onActivityResult. Does it work if you disable configuration change?

fn-jt commented 3 years ago

Hey @FeiChen-okta, sorry we didn't clarify, but configuration changes aren't our problem. Our app is getting killed just on the redirect to Chrome (w/o any configuration change). Maybe this was the wrong issue to ask this question. I had assumed that the crux of the problem was just the app getting killed since I believe coreform's statement, "the Activity it was started from destroyed and the ResultCallback orphaned, never to be called back and any ActivityResult expectations the Okta lib had will not eventuate" would apply to any situation where the app is backgrounded and the calling Activity destroyed.

I meant that I put breakpoints in the code you mentioned(RedirectActivity, OktaAuthenticationActivity, OktaResultFragment, etc), and none of those breakpoints are getting triggered on the first login and redirect back to our app. I see that in the SDK manifest, the RedirectActivity is supposed to start in response to a data intent filter on our schema, so this is puzzling.

FeiChen-okta commented 3 years ago

Hi @fn-jt It seems your device or something else is causing the app to close.

I would first try the following:

  1. Check the following on your emulator or device. Go to Settings -> Developer options -> Don't keep activities is off.

    Screen Shot 2021-06-02 at 12 15 34 PM
  2. Check the events log. adb logcat -b events See if your activity is being destroyed during the sign in flow. You should see something like wm_destroy_activity: {your app package}

  3. Try to reproduce this by using the sample app https://github.com/okta/samples-android/tree/master/sign-in-kotlin

wyland commented 3 years ago

@FeiChen-okta

I responded to our open support ticket (01123367) with Marcus regarding our logcat logs, we indeed are seeing are activity get destroyed - no errors in the log either. I also confirmed that "Developer -> Don't keep Activities" is disabled before running.

coreform commented 3 years ago

@wyland your Activity is probably doing too much, making it the main candidate for killing when the OS needs more memory. Try making a vcery lightweight shim Activity to handle the Okta stuff in isolation and see how that goes, I expect your bulky activity will still get killed while the lightweight one has more chance to stick around.

coreform commented 3 years ago

Back to the topic of config changes vs this lib: signOut is behaving particularly oddly in 1.0.20, if calling from an activity that is in landscape the okta lib is forcing the app into portrait and that config change severs the callback from Okta lib, either resulting in no signal to the callback or sometimes an odd crash:

2021-07-27 11:03:06.956 8525-8525/edu.monash.monashmobile E/AndroidRuntime: FATAL EXCEPTION: main Process: <redacted>, PID: 8525 android.content.ActivityNotFoundException: No Activity found to handle Intent { act=android.intent.action.VIEW pkg=com.android.chrome (has extras) } at android.app.Instrumentation.checkStartActivityResult(Instrumentation.java:2067) at android.app.Instrumentation.execStartActivity(Instrumentation.java:1727) at android.app.Activity.startActivityForResult(Activity.java:5314) at android.app.Activity.startActivityForResult(Activity.java:5272) at android.app.Activity.startActivity(Activity.java:5658) at android.app.Activity.startActivity(Activity.java:5611) at com.okta.oidc.OktaAuthenticationActivity.onServiceConnected(OktaAuthenticationActivity.java:281) at com.okta.oidc.ServiceConnection.onCustomTabsServiceConnected(ServiceConnection.java:44) at androidx.browser.customtabs.CustomTabsServiceConnection.onServiceConnected(CustomTabsServiceConnection.java:57) at android.app.LoadedApk$ServiceDispatcher.doConnected(LoadedApk.java:1978) at android.app.LoadedApk$ServiceDispatcher$RunConnection.run(LoadedApk.java:2010) at android.os.Handler.handleCallback(Handler.java:938) at android.os.Handler.dispatchMessage(Handler.java:99) at android.os.Looper.loop(Looper.java:223) at android.app.ActivityThread.main(ActivityThread.java:7664) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)

js-bonin commented 2 years ago

We are having the same issue here, any news on this?

JayNewstrom commented 2 years ago

We've not been able to reproduce this. Would someone be willing to see if you can recreate this in our new SDK? https://github.com/okta/okta-mobile-kotlin

JayNewstrom commented 2 years ago

Please reopen with steps to recreate.