Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import com.auth0.android.result.Credentials
import com.auth0.android.result.SSOCredentials
import com.auth0.android.result.UserProfile
import com.auth0.android.util.Clock
import java.util.*
import java.util.concurrent.atomic.AtomicBoolean

/**
* Base class meant to abstract common logic across Credentials Manager implementations.
Expand Down Expand Up @@ -428,18 +428,42 @@ public abstract class BaseCredentialsManager internal constructor(

internal inline fun <T> runCatchingOnExecutor(
callback: Callback<T, CredentialsManagerException>,
block: () -> Unit
block: (Callback<T, CredentialsManagerException>) -> Unit
) {
val singleShotCallback = SingleShotCallback(callback)
try {
block()
block(singleShotCallback)
} catch (t: Throwable) {
if (t is VirtualMachineError || t is ThreadDeath || t is LinkageError) {
throw t
}
Log.e("BaseCredentialsManager", "Unexpected error in executor block", t)
callback.onFailure(
singleShotCallback.onFailure(
CredentialsManagerException(CredentialsManagerException.Code.UNKNOWN_ERROR, t)
)
}
}

/**
* Delegates to [delegate] at most once. Any [onSuccess]/[onFailure] call after the first is
* silently dropped, upholding the fire-once contract of [Callback] even when the executor block
* both invokes the callback and later throws.
*/
internal class SingleShotCallback<T>(
private val delegate: Callback<T, CredentialsManagerException>
) : Callback<T, CredentialsManagerException> {
private val handled = AtomicBoolean(false)

override fun onSuccess(result: T) {
if (handled.compareAndSet(false, true)) {
delegate.onSuccess(result)
}
}

override fun onFailure(error: CredentialsManagerException) {
if (handled.compareAndSet(false, true)) {
delegate.onFailure(error)
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting
callback: Callback<SSOCredentials, CredentialsManagerException>
) {
serialExecutor.execute {
runCatchingOnExecutor(callback) {
runCatchingOnExecutor(callback) { callback ->
// IPSIE session_expiry: enforce the upstream-IdP session ceiling before exchanging the
// refresh token, so the SSO exchange is never used to outlive the session.
if (isSessionExpired(storage.retrieveString(KEY_ID_TOKEN))) {
Expand Down Expand Up @@ -220,11 +220,11 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting
parameters,
object : Callback<SSOCredentials, CredentialsManagerException> {
override fun onSuccess(result: SSOCredentials) {
continuation.resume(result)
if (continuation.isActive) continuation.resume(result)
}

override fun onFailure(error: CredentialsManagerException) {
continuation.resumeWithException(error)
if (continuation.isActive) continuation.resumeWithException(error)
}
})
}
Expand Down Expand Up @@ -329,11 +329,11 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting
forceRefresh,
object : Callback<Credentials, CredentialsManagerException> {
override fun onSuccess(result: Credentials) {
continuation.resume(result)
if (continuation.isActive) continuation.resume(result)
}

override fun onFailure(error: CredentialsManagerException) {
continuation.resumeWithException(error)
if (continuation.isActive) continuation.resumeWithException(error)
}
})
}
Expand Down Expand Up @@ -366,11 +366,11 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting
audience, scope, minTtl, parameters, headers,
object : Callback<APICredentials, CredentialsManagerException> {
override fun onSuccess(result: APICredentials) {
continuation.resume(result)
if (continuation.isActive) continuation.resume(result)
}

override fun onFailure(error: CredentialsManagerException) {
continuation.resumeWithException(error)
if (continuation.isActive) continuation.resumeWithException(error)
}
}
)
Expand Down Expand Up @@ -466,7 +466,7 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting
callback: Callback<Credentials, CredentialsManagerException>
) {
serialExecutor.execute {
runCatchingOnExecutor(callback) {
runCatchingOnExecutor(callback) { callback ->
val accessToken = storage.retrieveString(KEY_ACCESS_TOKEN)
val refreshToken = storage.retrieveString(KEY_REFRESH_TOKEN)
val idToken = storage.retrieveString(KEY_ID_TOKEN)
Expand Down Expand Up @@ -607,7 +607,7 @@ public class CredentialsManager @VisibleForTesting(otherwise = VisibleForTesting
) {

serialExecutor.execute {
runCatchingOnExecutor(callback) {
runCatchingOnExecutor(callback) { callback ->
// IPSIE session_expiry: enforce the upstream-IdP session ceiling before serving cached
// API credentials or exchanging the refresh token, so the session is never extended past it.
if (isSessionExpired(storage.retrieveString(KEY_ID_TOKEN))) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT
callback: Callback<SSOCredentials, CredentialsManagerException>
) {
serialExecutor.execute {
runCatchingOnExecutor(callback) {
runCatchingOnExecutor(callback) { callback ->
val existingCredentials: Credentials = try {
getExistingCredentials()
} catch (exception: CredentialsManagerException) {
Expand Down Expand Up @@ -330,11 +330,11 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT
parameters,
object : Callback<SSOCredentials, CredentialsManagerException> {
override fun onSuccess(result: SSOCredentials) {
continuation.resume(result)
if (continuation.isActive) continuation.resume(result)
}

override fun onFailure(error: CredentialsManagerException) {
continuation.resumeWithException(error)
if (continuation.isActive) continuation.resumeWithException(error)
}
})
}
Expand Down Expand Up @@ -459,11 +459,11 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT
forceRefresh,
object : Callback<Credentials, CredentialsManagerException> {
override fun onSuccess(result: Credentials) {
continuation.resume(result)
if (continuation.isActive) continuation.resume(result)
}

override fun onFailure(error: CredentialsManagerException) {
continuation.resumeWithException(error)
if (continuation.isActive) continuation.resumeWithException(error)
}
})
}
Expand Down Expand Up @@ -500,11 +500,11 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT
headers,
object : Callback<APICredentials, CredentialsManagerException> {
override fun onSuccess(result: APICredentials) {
continuation.resume(result)
if (continuation.isActive) continuation.resume(result)
}

override fun onFailure(error: CredentialsManagerException) {
continuation.resumeWithException(error)
if (continuation.isActive) continuation.resumeWithException(error)
}
})
}
Expand Down Expand Up @@ -793,7 +793,7 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT
callback: Callback<Credentials, CredentialsManagerException>
) {
serialExecutor.execute {
runCatchingOnExecutor(callback) {
runCatchingOnExecutor(callback) { callback ->
val encryptedEncoded = storage.retrieveString(KEY_CREDENTIALS)
if (encryptedEncoded.isNullOrBlank()) {
callback.onFailure(CredentialsManagerException.NO_CREDENTIALS)
Expand Down Expand Up @@ -960,7 +960,7 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT
callback: Callback<APICredentials, CredentialsManagerException>
) {
serialExecutor.execute {
runCatchingOnExecutor(callback) {
runCatchingOnExecutor(callback) { callback ->
// IPSIE session_expiry: enforce the upstream-IdP session ceiling before serving
// cached API credentials or exchanging the refresh token. The ceiling is read from
// the value persisted at login (KEY_SESSION_EXPIRY) so it holds even though the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import org.mockito.Mockito
import org.mockito.MockitoAnnotations
import org.mockito.kotlin.KArgumentCaptor
import org.mockito.kotlin.any
import org.mockito.kotlin.anyOrNull
import org.mockito.kotlin.argumentCaptor
import org.mockito.kotlin.eq
import org.mockito.kotlin.mock
Expand Down Expand Up @@ -2644,6 +2645,69 @@ public class CredentialsManagerTest {
MatcherAssert.assertThat(exception.cause, Is.`is`(error))
}

// a Throwable escaping the executor block AFTER onSuccess already fired must not
// trigger a second terminal callback. runCatchingOnExecutor wraps the callback in a single-shot
// guard, so onFailure is dropped. Without it, the coroutine bridge would resume twice
// ("Already resumed") and crash the process.
@Test
public fun shouldNotInvokeCallbackTwiceWhenConsumerThrowsAfterSuccessOnGetCredentials() {
Mockito.`when`(storage.retrieveString("com.auth0.id_token")).thenReturn("idToken")
Mockito.`when`(storage.retrieveString("com.auth0.access_token")).thenReturn("accessToken")
Mockito.`when`(storage.retrieveString("com.auth0.refresh_token")).thenReturn("refreshToken")
Mockito.`when`(storage.retrieveString("com.auth0.token_type")).thenReturn("type")
val expirationTime = CredentialsMock.ONE_HOUR_AHEAD_MS
Mockito.`when`(storage.retrieveLong("com.auth0.expires_at")).thenReturn(expirationTime)
Mockito.`when`(storage.retrieveLong("com.auth0.cache_expires_at")).thenReturn(expirationTime)
Mockito.`when`(storage.retrieveString("com.auth0.scope")).thenReturn("scope")

var successCount = 0
var failureCount = 0
manager.getCredentials(object : Callback<Credentials, CredentialsManagerException> {
override fun onSuccess(result: Credentials) {
successCount++
// Simulate a consumer/bridge that throws right after receiving the result.
throw IllegalStateException("Simulated post-resume failure in executor block")
}

override fun onFailure(error: CredentialsManagerException) {
failureCount++
}
})

MatcherAssert.assertThat(successCount, Is.`is`(1))
MatcherAssert.assertThat(failureCount, Is.`is`(0))
}

// Layer B (coroutine bridge) regression: if the underlying callback getCredentials fires a
// terminal callback twice (onSuccess then onFailure), the suspendCancellableCoroutine bridge
// must resume the continuation only once. Without the `if (continuation.isActive)` guard the
// second resume throws IllegalStateException("Already resumed") on the executor thread and
// crashes the process. Here the first resume wins, so awaitCredentials returns normally.
@Test
@ExperimentalCoroutinesApi
public fun shouldNotResumeContinuationTwiceWhenCallbackFiresTwiceOnAwaitCredentials(): Unit =
runTest {
val credentials = CredentialsMock.create(
"idToken", "accessToken", "type", "refreshToken",
Date(CredentialsMock.ONE_HOUR_AHEAD_MS), "scope"
)
Mockito.doAnswer { invocation ->
@Suppress("UNCHECKED_CAST")
val cb =
invocation.arguments.last() as Callback<Credentials, CredentialsManagerException>
// Simulate a double terminal callback reaching the bridge.
cb.onSuccess(credentials)
cb.onFailure(CredentialsManagerException(CredentialsManagerException.Code.UNKNOWN_ERROR))
null
}.`when`(manager).getCredentials(
anyOrNull(), any(), any(), any(), any(), any()
)

val result = manager.awaitCredentials()
MatcherAssert.assertThat(result, Is.`is`(Matchers.notNullValue()))
MatcherAssert.assertThat(result.accessToken, Is.`is`("accessToken"))
}

@After
public fun tearDown() {
DPoPUtil.keyStore = DPoPKeyStore()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4404,6 +4404,66 @@ public class SecureCredentialsManagerTest {
MatcherAssert.assertThat(exception.cause, Is.`is`(error))
}

// a Throwable escaping the executor block AFTER onSuccess already fired must not
// trigger a second terminal callback. runCatchingOnExecutor wraps the callback in a single-shot
// guard, so onFailure is dropped. Without it, the awaitCredentials coroutine bridge would resume
// twice ("Already resumed") on the serial executor thread and crash the process.
@Test
public fun shouldNotInvokeCallbackTwiceWhenConsumerThrowsAfterSuccessOnGetCredentials() {
Mockito.`when`(localAuthenticationManager.authenticate()).then {
localAuthenticationManager.resultCallback.onSuccess(true)
}
val expiresAt = Date(CredentialsMock.CURRENT_TIME_MS + ONE_HOUR_SECONDS * 1000)
insertTestCredentials(true, true, true, expiresAt, "scope")

var successCount = 0
var failureCount = 0
manager.getCredentials(object : Callback<Credentials, CredentialsManagerException> {
override fun onSuccess(result: Credentials) {
successCount++
// Simulate a consumer/bridge that throws right after receiving the result.
throw IllegalStateException("Simulated post-resume failure in executor block")
}

override fun onFailure(error: CredentialsManagerException) {
failureCount++
}
})

MatcherAssert.assertThat(successCount, Is.`is`(1))
MatcherAssert.assertThat(failureCount, Is.`is`(0))
}

// Layer B (coroutine bridge) regression: if the underlying callback getCredentials fires a
// terminal callback twice (onSuccess then onFailure), the suspendCancellableCoroutine bridge
// must resume the continuation only once. Without the `if (continuation.isActive)` guard the
// second resume throws IllegalStateException("Already resumed") on the serial executor thread
// and crashes the process. Here the first resume wins, so awaitCredentials returns normally.
@Test
@ExperimentalCoroutinesApi
public fun shouldNotResumeContinuationTwiceWhenCallbackFiresTwiceOnAwaitCredentials(): Unit =
runTest {
val credentials = CredentialsMock.create(
"idToken", "accessToken", "type", "refreshToken",
Date(CredentialsMock.ONE_HOUR_AHEAD_MS), "scope"
)
Mockito.doAnswer { invocation ->
@Suppress("UNCHECKED_CAST")
val cb =
invocation.arguments.last() as Callback<Credentials, CredentialsManagerException>
// Simulate a double terminal callback reaching the bridge.
cb.onSuccess(credentials)
cb.onFailure(CredentialsManagerException(CredentialsManagerException.Code.UNKNOWN_ERROR))
null
}.`when`(manager).getCredentials(
anyOrNull(), any(), any(), any(), any(), any()
)

val result = manager.awaitCredentials()
MatcherAssert.assertThat(result, Is.`is`(Matchers.notNullValue()))
MatcherAssert.assertThat(result.accessToken, Is.`is`("accessToken"))
}

// Verifies the outer runCatchingOnExecutor safety net in getSsoCredentials.
// The exception is thrown from getExistingCredentials() → storage.retrieveString()
// before any inner try/catch is reached.
Expand Down
Loading