search

okta / okta-oidc-android

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

Exception when calling WebAuthClient.cancel() #318

Closed idomo1 closed 2 years ago

idomo1 commented 2 years ago

Describe the bug?

When WebAuthClient.cancel() is called on some Android versions, the onError callback is invoked. I've found this when testing on emulators. I don't have the devices on hand to test whether it happens on real devices as well.

What is expected to happen?

The onCancel callback is invoked.

What is the actual behavior?

The following exception:

Android 8, 8.1, 9, 10, 12

AuthorizationException: {"type":0,"code":0,"errorDescription":"Socket closed"}
        at com.okta.oidc.net.request.ConfigurationRequest.executeRequest(ConfigurationRequest.java:68)
        at com.okta.oidc.clients.AuthAPI.obtainNewConfiguration(AuthAPI.java:101)
        at com.okta.oidc.clients.web.SyncWebAuthClientImpl.signIn(SyncWebAuthClientImpl.java:222)
        at com.okta.oidc.clients.web.WebAuthClientImpl.lambda$signIn$3$com-okta-oidc-clients-web-WebAuthClientImpl(WebAuthClientImpl.java:149)
        at com.okta.oidc.clients.web.WebAuthClientImpl$$ExternalSyntheticLambda1.run(Unknown Source:6)
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:462)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:919)
     Caused by: java.net.SocketException: Socket closed
        at java.net.AbstractPlainSocketImpl.doConnect(AbstractPlainSocketImpl.java:394)
        at java.net.AbstractPlainSocketImpl.connectToAddress(AbstractPlainSocketImpl.java:230)
        at java.net.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:212)
        at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:436)
        at java.net.Socket.connect(Socket.java:621)
        at com.android.okhttp.internal.Platform.connectSocket(Platform.java:182)
        at com.android.okhttp.internal.io.RealConnection.connectSocket(RealConnection.java:145)
        at com.android.okhttp.internal.io.RealConnection.connect(RealConnection.java:116)
        at com.android.okhttp.internal.http.StreamAllocation.findConnection(StreamAllocation.java:186)
        at com.android.okhttp.internal.http.StreamAllocation.findHealthyConnection(StreamAllocation.java:128)
        at com.android.okhttp.internal.http.StreamAllocation.newStream(StreamAllocation.java:97)
        at com.android.okhttp.internal.http.HttpEngine.connect(HttpEngine.java:289)
        at com.android.okhttp.internal.http.HttpEngine.sendRequest(HttpEngine.java:232)
        at com.android.okhttp.internal.huc.HttpURLConnectionImpl.execute(HttpURLConnectionImpl.java:465)
        at com.android.okhttp.internal.huc.HttpURLConnectionImpl.connect(HttpURLConnectionImpl.java:131)
        at com.android.okhttp.internal.huc.DelegatingHttpsURLConnection.connect(DelegatingHttpsURLConnection.java:90)
        at com.android.okhttp.internal.huc.HttpsURLConnectionImpl.connect(HttpsURLConnectionImpl.java:30)
        at com.okta.oidc.net.HttpClientImpl.connect(HttpClientImpl.java:113)
        at com.okta.oidc.net.request.BaseRequest.openConnection(BaseRequest.java:58)
        at com.okta.oidc.net.request.ConfigurationRequest.executeRequest(ConfigurationRequest.java:60)
        at com.okta.oidc.clients.AuthAPI.obtainNewConfiguration(AuthAPI.java:101) 
        at com.okta.oidc.clients.web.SyncWebAuthClientImpl.signIn(SyncWebAuthClientImpl.java:222) 
        at com.okta.oidc.clients.web.WebAuthClientImpl.lambda$signIn$3$com-okta-oidc-clients-web-WebAuthClientImpl(WebAuthClientImpl.java:149) 
        at com.okta.oidc.clients.web.WebAuthClientImpl$$ExternalSyntheticLambda1.run(Unknown Source:6) 
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:462) 
        at java.util.concurrent.FutureTask.run(FutureTask.java:266) 
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167) 
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) 
        at java.lang.Thread.run(Thread.java:919)

Or sometimes

AuthorizationException: {"type":0,"code":0,"errorDescription":"Canceled"}
        at com.okta.oidc.net.request.ConfigurationRequest.executeRequest(ConfigurationRequest.java:68)
        at com.okta.oidc.clients.AuthAPI.obtainNewConfiguration(AuthAPI.java:101)
        at com.okta.oidc.clients.web.SyncWebAuthClientImpl.signIn(SyncWebAuthClientImpl.java:228)
        at com.okta.oidc.clients.web.WebAuthClientImpl.lambda$signIn$3$com-okta-oidc-clients-web-WebAuthClientImpl(WebAuthClientImpl.java:149)
        at com.okta.oidc.clients.web.WebAuthClientImpl$$ExternalSyntheticLambda1.run(Unknown Source:6)
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:458)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:764)
     Caused by: java.io.IOException: Canceled
        at com.okta.oidc.net.request.BaseRequest.openConnection(BaseRequest.java:60)
        at com.okta.oidc.net.request.ConfigurationRequest.executeRequest(ConfigurationRequest.java:60)

Reproduction Steps?

val client: WebAuthClient = buildWebClient(activity)
client.registerCallback(...)
client.signIn(activity, null)

GlobalScope.launch {
                // Delay simulates human reaction time. Immediately calling cancel causes other errors.
               // May need to adjust this to make sure the call happens before the custom tab is launched.
                delay(300L)
                cancel()
            }

Additional Information?

No response

SDK Version

1.2.5

Build Information

Android 8, 8.1, 9, 10, 12

JayNewstrom commented 2 years ago

Thanks for the report, I'll take a look and see if I can reproduce.

JayNewstrom commented 2 years ago

It looks like this has less to do with the android version, and more to do with the current cache state. We cache the .well-known/openid-configuration, and if that cache exists, it takes much less time to get the value to perform the rest of the flow. So it's getting to the point where we call the cancel callback before your delay is over.

That being said. What is your use case for this? I'm working on a new kotlin/coroutine based SDK, and I'd like to make sure we're handling this properly before a 1.0 release.

As for a workaround in the current SDK, I'd suggest calling unregisterCallback() before cancel(), and calling your centralized cancel logic afterwards.

JayNewstrom commented 2 years ago

The biggest issue here is cancel doesn't really cancel the flow, and it can't due to the way activities work in Android (we launch an activity we don't have control over, ie Chrome).

Cancel really only makes sense for cancelling the result handling due to this issue.

idomo1 commented 2 years ago

Thanks for looking into this. The use case is to let user press back to cancel the login flow in the time between client.signIn() and Chrome opening. It's just a nice to have, not something that's essential.

The problem I can see with just unregistering the callback is that cancel isn't guaranteed to work, so without the callback we don't know if it really was cancelled (please correct me if I'm wrong). Something that might work is to just add logic to treat the next onError as a cancel after calling cancel().

JayNewstrom commented 2 years ago

I see, that use case is correctly accounted for in the new SDK.

So unfortunately calling cancel doesn't actually cancel the network request in this SDK, but it will in the new SDK! That being said, we can check for if the flow has been cancelled via cancel when the network request ends up failing, and properly report the cancellation. I've got a proof of concept doing just that locally. But I'll figure out if it's the right thing to do, and finish it up next week.

All that being said. I still think the "work around" I outlined above is the best approach, so the user doesn't have to wait for the network call to complete before you let them move on.

JayNewstrom commented 2 years ago

@idomo1 fixed and released as 1.2.6.