okta / okta-oidc-android

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

Request body empty signIn call - InputStreamRequestBody does not reset `pos` after `writeAll` call #264

Closed scottfennell closed 2 years ago

scottfennell commented 2 years ago

I am observing an issue that appears to be caused by the Android Studio profiler (com.android.tools.profiler.agent.okhttp.OkHttp3Interceptor). When this interceptor is present in the OkHttp interceptor chain, it will call InputStreamRequestBody#writeAll which reads all of the data from the ByteArrayInputStream and leaves its "pos" value set to the final index of the array. When the InputStreamRequestBody#writeAll is called again from the CallServerInterceptor the underlying IS appears to be empty and no data is written to the socket.

JayNewstrom commented 2 years ago

Looks like a duplicate of https://github.com/okta/okta-oidc-android/issues/259

The best way to fix this is via an interceptor in your app that buffers the request. Or to disable Android studio profiler network section.

scottfennell commented 2 years ago

Disabling the profiler does not appear to be an option, this is an issue with builds outside of Android Studio as well. This appears to be device specific. Pixel 2, android 11 has this issue, samsung note 20 does not.

I am not sure the mechanism that causes the interceptor to get added, but I do know that I am not seeing issues with other requests.

JayNewstrom commented 2 years ago

@scottfennell can you confirm you're seeing this issue with okta-auth-java, not okta-oidc-android? Can you provide a code snippet of the call that triggers this error?

scottfennell commented 2 years ago

Here is the request that I am observing that is failing, however we are seeing a lot of failed requests when refreshing tokens - I think the ultimate issue is in the okta-http-okhttp dependency.

        val authClient = AuthenticationClients.builder()
            .setOrgUrl(context.getString(endpoint))
            .build()
        val authenticationResult = suspendCancellableCoroutine<AuthenticationResponse?> { cont ->
            try {
                val request = authClient.instantiate(AuthenticationRequest::class.java)
                    .setUsername(username)
                    .setPassword(password.toCharArray())
                    .setRelayState(null)
                authClient.authenticate(
                    request,
                    null,
                    object : AuthenticationStateHandlerAdapter() {...}
                )
            } catch () {}
        }
JayNewstrom commented 2 years ago

Thanks. This is indeed our okta-auth-java SDK, which uses networking from our okta-commons-java SDK. https://github.com/okta/okta-commons-java/blob/master/http/okhttp/src/main/java/com/okta/commons/http/okhttp/OkHttpRequestExecutor.java#L173

I pinged our maintainer about it, and he said he'd take a look soon. But also feel free to open an issue in that repo if you'd like.

JayNewstrom commented 2 years ago

I opened a PR with the fix here: https://github.com/okta/okta-commons-java/pull/57#pullrequestreview-740737144

JayNewstrom commented 2 years ago

PR is merged, and should be released soon.