diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/data/platform/processor/AuthenticatorBridgeProcessorImpl.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/platform/processor/AuthenticatorBridgeProcessorImpl.kt index af11a08555..91293b776b 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/data/platform/processor/AuthenticatorBridgeProcessorImpl.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/platform/processor/AuthenticatorBridgeProcessorImpl.kt @@ -4,6 +4,7 @@ import android.content.Context import android.os.Build import android.os.IInterface import android.os.RemoteCallbackList +import android.os.RemoteException import androidx.core.net.toUri import com.bitwarden.authenticatorbridge.IAuthenticatorBridgeService import com.bitwarden.authenticatorbridge.IAuthenticatorBridgeServiceCallback @@ -144,17 +145,25 @@ class AuthenticatorBridgeProcessorImpl( } /** - * This function mirrors the hidden "RemoteCallbackList.broadcast" function. + * Invokes [action] on each registered callback, similar to the hidden + * "RemoteCallbackList.broadcast" function. A failure in one callback is logged and does not + * prevent the remaining callbacks from being invoked. */ @Suppress("TooGenericExceptionCaught") private fun RemoteCallbackList.forEach(action: (T) -> Unit) { val count = this.beginBroadcast() try { - for (index in 0..count) { - action(this.getBroadcastItem(index)) + for (index in 0 until count) { + try { + action(this.getBroadcastItem(index)) + } catch (e: RemoteException) { + // Callback process died; RemoteCallbackList unregisters it via binderDied. + Timber.w(e, "Bridge callback failed") + } catch (e: RuntimeException) { + // Catch broadly so one misbehaving callback cannot block the rest. + Timber.e(e, "Bridge callback threw unexpectedly") + } } - } catch (e: Exception) { - // Broadcast failed } finally { this.finishBroadcast() } diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/data/platform/processor/AuthenticatorBridgeProcessorTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/data/platform/processor/AuthenticatorBridgeProcessorTest.kt index 1fc10883b1..51efd28541 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/data/platform/processor/AuthenticatorBridgeProcessorTest.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/platform/processor/AuthenticatorBridgeProcessorTest.kt @@ -5,6 +5,7 @@ import android.content.Intent import android.net.Uri import android.os.Build import android.os.RemoteCallbackList +import android.os.RemoteException import com.bitwarden.authenticatorbridge.IAuthenticatorBridgeService import com.bitwarden.authenticatorbridge.IAuthenticatorBridgeServiceCallback import com.bitwarden.authenticatorbridge.model.AddTotpLoginItemData @@ -25,6 +26,7 @@ import com.bitwarden.ui.platform.util.getTotpDataOrNull import com.x8bit.bitwarden.data.auth.manager.AddTotpItemFromAuthenticatorManagerImpl import com.x8bit.bitwarden.data.platform.repository.AuthenticatorBridgeRepository import com.x8bit.bitwarden.data.platform.util.createAddTotpItemFromAuthenticatorIntent +import io.mockk.clearConstructorMockk import io.mockk.coEvery import io.mockk.coVerify import io.mockk.every @@ -259,6 +261,133 @@ class AuthenticatorBridgeProcessorTest { lastAccountsSync = null } + @Test + fun `syncAccounts should only fetch callbacks within the broadcast count`() { + clearConstructorMockk(RemoteCallbackList::class, answers = false) + val sharedAccountData = mockk() + val expected = mockk() + + every { + authenticatorBridgeRepository.authenticatorSyncSymmetricKey + } returns SYMMETRIC_KEY.symmetricEncryptionKey.byteArray + + coEvery { + authenticatorBridgeRepository.getSharedAccounts() + } returns sharedAccountData + + every { + sharedAccountData.encrypt(SYMMETRIC_KEY) + } returns expected.asSuccess() + + getDefaultBinder().syncAccounts() + + assertEquals(expected, lastAccountsSync) + verify(exactly = 1) { + anyConstructed>() + .getBroadcastItem(0) + } + + verify(exactly = 0) { + anyConstructed>() + .getBroadcastItem(1) + } + + verify(exactly = 1) { + anyConstructed>() + .finishBroadcast() + } + } + + @Test + fun `syncAccounts should still notify later callbacks when an earlier one fails`() { + val deadCallback = object : IAuthenticatorBridgeServiceCallback.Stub() { + override fun onAccountsSync(data: EncryptedSharedAccountData?) { + throw RemoteException("process died") + } + } + + every { + anyConstructed>() + .beginBroadcast() + } returns 2 + + every { + anyConstructed>() + .getBroadcastItem(0) + } returns deadCallback + + every { + anyConstructed>() + .getBroadcastItem(1) + } returns serviceCallback + + val sharedAccountData = mockk() + val expected = mockk() + + every { + authenticatorBridgeRepository.authenticatorSyncSymmetricKey + } returns SYMMETRIC_KEY.symmetricEncryptionKey.byteArray + + coEvery { + authenticatorBridgeRepository.getSharedAccounts() + } returns sharedAccountData + + every { + sharedAccountData.encrypt(SYMMETRIC_KEY) + } returns expected.asSuccess() + + getDefaultBinder().syncAccounts() + + assertEquals(expected, lastAccountsSync) + } + + @Test + fun `syncAccounts should still notify later callbacks when one throws unexpectedly`() { + val faultyCallback = object : IAuthenticatorBridgeServiceCallback.Stub() { + override fun onAccountsSync(data: EncryptedSharedAccountData?) { + throw IllegalStateException("unexpected failure") + } + } + + every { + anyConstructed>() + .beginBroadcast() + } returns 2 + + every { + anyConstructed>() + .getBroadcastItem(0) + } returns faultyCallback + + every { + anyConstructed>() + .getBroadcastItem(1) + } returns serviceCallback + + val sharedAccountData = mockk() + val expected = mockk() + + every { + authenticatorBridgeRepository.authenticatorSyncSymmetricKey + } returns SYMMETRIC_KEY.symmetricEncryptionKey.byteArray + + coEvery { + authenticatorBridgeRepository.getSharedAccounts() + } returns sharedAccountData + + every { + sharedAccountData.encrypt(SYMMETRIC_KEY) + } returns expected.asSuccess() + + getDefaultBinder().syncAccounts() + + assertEquals(expected, lastAccountsSync) + verify(exactly = 1) { + anyConstructed>() + .finishBroadcast() + } + } + @Test fun `syncAccounts when symmetricEncryptionKeyData is null should do nothing`() { every { authenticatorBridgeRepository.authenticatorSyncSymmetricKey } returns null