From c6f7a47c9acd74fc3f2d0f33142a35550f0e29b8 Mon Sep 17 00:00:00 2001 From: David Perez Date: Tue, 21 Jul 2026 17:17:19 +0000 Subject: [PATCH] PM-40278: bug: Freeze public key and fingerprint during auth request and validate against updates (#7166) --- .../auth/manager/AuthRequestManagerImpl.kt | 59 ++++-- .../auth/manager/AuthRequestManagerTest.kt | 176 +++++++++++++++--- 2 files changed, 194 insertions(+), 41 deletions(-) diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerImpl.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerImpl.kt index 2a6ccd31b0..22855876f2 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerImpl.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerImpl.kt @@ -27,7 +27,9 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.flow import kotlinx.coroutines.isActive import java.time.Clock +import java.time.Instant import javax.inject.Singleton +import kotlin.time.Duration.Companion.milliseconds private const val PASSWORDLESS_NOTIFICATION_TIMEOUT_MILLIS: Long = 15L * 60L * 1_000L private const val PASSWORDLESS_NOTIFICATION_RETRY_INTERVAL_MILLIS: Long = 4L * 1_000L @@ -154,32 +156,49 @@ class AuthRequestManagerImpl( } } + @Suppress("LongMethod") private fun getAuthRequest( initialRequest: suspend () -> AuthRequestUpdatesResult, ): Flow = flow { - val result = initialRequest() - emit(result) - if (result is AuthRequestUpdatesResult.Error) return@flow + val initialResult = initialRequest() + emit(initialResult) + if (initialResult !is AuthRequestUpdatesResult.Update) return@flow + val initialAuthRequest = initialResult.authRequest var isComplete = false while (currentCoroutineContext().isActive && !isComplete) { - delay(PASSWORDLESS_APPROVER_INTERVAL_MILLIS) - val updateResult = result as AuthRequestUpdatesResult.Update + delay(PASSWORDLESS_APPROVER_INTERVAL_MILLIS.milliseconds) authRequestsService - .getAuthRequest(result.authRequest.id) - .map { request -> - AuthRequest( - id = request.id, - publicKey = request.publicKey, - platform = request.platform, - ipAddress = request.ipAddress, - key = request.key, - masterPasswordHash = request.masterPasswordHash, - creationDate = request.creationDate, - responseDate = request.responseDate, - requestApproved = request.requestApproved ?: false, - originUrl = request.originUrl, - fingerprint = updateResult.authRequest.fingerprint, - ) + .getAuthRequest(requestId = initialResult.authRequest.id) + .flatMap { request -> + getFingerprintPhrase(publicKey = request.publicKey).map { fingerprint -> + val isRequestApproved: Boolean + val responseDate: Instant? + if (fingerprint == initialAuthRequest.fingerprint) { + // Fingerprint is valid, so we process the valid response. + isRequestApproved = request.requestApproved ?: false + responseDate = request.responseDate + } else { + // We cannot gaurentee the authenticity of the response, so we will + // hard-decline the request. + isRequestApproved = false + responseDate = clock.instant() + } + AuthRequest( + id = request.id, + platform = request.platform, + ipAddress = request.ipAddress, + key = request.key, + masterPasswordHash = request.masterPasswordHash, + creationDate = request.creationDate, + originUrl = request.originUrl, + responseDate = responseDate, + requestApproved = isRequestApproved, + // The PublicKey and Fingerprint should be frozen in place to + // ensure no funny-business happens between multiple requests. + publicKey = initialAuthRequest.publicKey, + fingerprint = initialAuthRequest.fingerprint, + ) + } } .fold( onFailure = { emit(AuthRequestUpdatesResult.Error(error = it)) }, diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerTest.kt index e73a0968b5..259edb131b 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerTest.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerTest.kt @@ -35,6 +35,7 @@ import org.junit.jupiter.api.Test import java.time.Clock import java.time.Instant import java.time.ZoneOffset +import kotlin.time.Duration.Companion.milliseconds @Suppress("LargeClass") class AuthRequestManagerTest { @@ -509,9 +510,11 @@ class AuthRequestManagerTest { coVerify(exactly = 1) { authRequestsService.getAuthRequests() - authSdkSource.getUserFingerprint(EMAIL, PUBLIC_KEY) authRequestsService.getAuthRequest(requestId = REQUEST_ID) } + coVerify(exactly = 2) { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } } @Suppress("MaxLineLength") @@ -546,9 +549,11 @@ class AuthRequestManagerTest { coVerify(exactly = 1) { authRequestsService.getAuthRequests() - authSdkSource.getUserFingerprint(EMAIL, PUBLIC_KEY) authRequestsService.getAuthRequest(requestId = REQUEST_ID) } + coVerify(exactly = 2) { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } } @Suppress("MaxLineLength") @@ -589,9 +594,11 @@ class AuthRequestManagerTest { coVerify(exactly = 1) { authRequestsService.getAuthRequests() - authSdkSource.getUserFingerprint(EMAIL, PUBLIC_KEY) authRequestsService.getAuthRequest(requestId = REQUEST_ID) } + coVerify(exactly = 2) { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } } @Suppress("MaxLineLength") @@ -631,9 +638,46 @@ class AuthRequestManagerTest { coVerify(exactly = 1) { authRequestsService.getAuthRequests() - authSdkSource.getUserFingerprint(EMAIL, PUBLIC_KEY) authRequestsService.getAuthRequest(requestId = REQUEST_ID) } + coVerify(exactly = 2) { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } + } + + @Suppress("MaxLineLength") + @Test + fun `getAuthRequestByFingerprintFlow should emit update then declined and cancel when polled fingerprint does not match initial fingerprint`() = + runTest { + val responseJsonOne = AuthRequestsResponseJson( + authRequests = listOf(AUTH_REQUESTS_RESPONSE_JSON_AUTH_RESPONSE), + ) + val expectedOne = AuthRequestUpdatesResult.Update(authRequest = AUTH_REQUEST) + val expectedTwo = AuthRequestUpdatesResult.Declined + coEvery { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } returns FINGER_PRINT.asSuccess() andThen "mismatchedFingerprint".asSuccess() + coEvery { authRequestsService.getAuthRequests() } returns responseJsonOne.asSuccess() + coEvery { + authRequestsService.getAuthRequest(requestId = REQUEST_ID) + } returns AUTH_REQUESTS_RESPONSE_JSON_AUTH_RESPONSE.asSuccess() + fakeAuthDiskSource.userState = SINGLE_USER_STATE + + repository + .getAuthRequestByFingerprintFlow(FINGER_PRINT) + .test { + assertEquals(expectedOne, awaitItem()) + assertEquals(expectedTwo, awaitItem()) + awaitComplete() + } + + coVerify(exactly = 1) { + authRequestsService.getAuthRequests() + authRequestsService.getAuthRequest(requestId = REQUEST_ID) + } + coVerify(exactly = 2) { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } } @Suppress("MaxLineLength") @@ -716,11 +760,9 @@ class AuthRequestManagerTest { awaitComplete() } - coVerify(exactly = 1) { - authSdkSource.getUserFingerprint(EMAIL, PUBLIC_KEY) - } coVerify(exactly = 2) { - authRequestsService.getAuthRequest(REQUEST_ID) + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + authRequestsService.getAuthRequest(requestId = REQUEST_ID) } } @@ -752,11 +794,9 @@ class AuthRequestManagerTest { awaitComplete() } - coVerify(exactly = 1) { - authSdkSource.getUserFingerprint(EMAIL, PUBLIC_KEY) - } coVerify(exactly = 2) { - authRequestsService.getAuthRequest(REQUEST_ID) + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + authRequestsService.getAuthRequest(requestId = REQUEST_ID) } } @@ -792,11 +832,9 @@ class AuthRequestManagerTest { awaitComplete() } - coVerify(exactly = 1) { - authSdkSource.getUserFingerprint(EMAIL, PUBLIC_KEY) - } coVerify(exactly = 2) { - authRequestsService.getAuthRequest(REQUEST_ID) + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + authRequestsService.getAuthRequest(requestId = REQUEST_ID) } } @@ -825,6 +863,101 @@ class AuthRequestManagerTest { } returns authRequestResponseOne andThen authRequestResponseTwo fakeAuthDiskSource.userState = SINGLE_USER_STATE + repository + .getAuthRequestByIdFlow(REQUEST_ID) + .test { + assertEquals(expectedOne, awaitItem()) + assertEquals(expectedTwo, awaitItem()) + cancelAndConsumeRemainingEvents() + } + + coVerify(exactly = 2) { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + authRequestsService.getAuthRequest(requestId = REQUEST_ID) + } + } + + @Suppress("MaxLineLength") + @Test + fun `getAuthRequestByIdFlow should emit update then declined and cancel when polled fingerprint does not match initial fingerprint`() = + runTest { + val expectedOne = AuthRequestUpdatesResult.Update(authRequest = AUTH_REQUEST) + val expectedTwo = AuthRequestUpdatesResult.Declined + coEvery { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } returns FINGER_PRINT.asSuccess() andThen "mismatchedFingerprint".asSuccess() + coEvery { + authRequestsService.getAuthRequest(requestId = REQUEST_ID) + } returns AUTH_REQUESTS_RESPONSE_JSON_AUTH_RESPONSE.asSuccess() + fakeAuthDiskSource.userState = SINGLE_USER_STATE + + repository + .getAuthRequestByIdFlow(REQUEST_ID) + .test { + assertEquals(expectedOne, awaitItem()) + assertEquals(expectedTwo, awaitItem()) + awaitComplete() + } + + coVerify(exactly = 2) { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + authRequestsService.getAuthRequest(requestId = REQUEST_ID) + } + } + + @Suppress("MaxLineLength") + @Test + fun `getAuthRequestByIdFlow should emit update then error and not cancel when getUserFingerprint fails during polling`() = + runTest { + val error = Throwable("Fail") + val expectedOne = AuthRequestUpdatesResult.Update(authRequest = AUTH_REQUEST) + val expectedTwo = AuthRequestUpdatesResult.Error(error = error) + coEvery { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } returns FINGER_PRINT.asSuccess() andThen error.asFailure() + coEvery { + authRequestsService.getAuthRequest(requestId = REQUEST_ID) + } returns AUTH_REQUESTS_RESPONSE_JSON_AUTH_RESPONSE.asSuccess() + fakeAuthDiskSource.userState = SINGLE_USER_STATE + + repository + .getAuthRequestByIdFlow(REQUEST_ID) + .test { + assertEquals(expectedOne, awaitItem()) + assertEquals(expectedTwo, awaitItem()) + cancelAndConsumeRemainingEvents() + } + + coVerify(exactly = 2) { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + authRequestsService.getAuthRequest(requestId = REQUEST_ID) + } + } + + @Suppress("MaxLineLength") + @Test + fun `getAuthRequestByIdFlow should emit update with initial publicKey when polled request returns a new publicKey with matching fingerprint`() = + runTest { + val newPublicKey = "newPublicKey" + val authRequestResponseOne = AUTH_REQUESTS_RESPONSE_JSON_AUTH_RESPONSE.asSuccess() + val authRequestResponseTwo = AUTH_REQUESTS_RESPONSE_JSON_AUTH_RESPONSE + .copy(publicKey = newPublicKey, requestApproved = false) + .asSuccess() + val expectedOne = AuthRequestUpdatesResult.Update(authRequest = AUTH_REQUEST) + val expectedTwo = AuthRequestUpdatesResult.Update( + authRequest = AUTH_REQUEST.copy(requestApproved = false), + ) + coEvery { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } returns FINGER_PRINT.asSuccess() + coEvery { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = newPublicKey) + } returns FINGER_PRINT.asSuccess() + coEvery { + authRequestsService.getAuthRequest(requestId = REQUEST_ID) + } returns authRequestResponseOne andThen authRequestResponseTwo + fakeAuthDiskSource.userState = SINGLE_USER_STATE + repository .getAuthRequestByIdFlow(REQUEST_ID) .test { @@ -834,10 +967,11 @@ class AuthRequestManagerTest { } coVerify(exactly = 1) { - authSdkSource.getUserFingerprint(EMAIL, PUBLIC_KEY) + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = newPublicKey) } coVerify(exactly = 2) { - authRequestsService.getAuthRequest(REQUEST_ID) + authRequestsService.getAuthRequest(requestId = REQUEST_ID) } } @@ -866,11 +1000,11 @@ class AuthRequestManagerTest { .getAuthRequestsWithUpdates() .test { assertEquals(expectedOne, awaitItem()) - advanceTimeBy(threeMinutes) + advanceTimeBy(threeMinutes.milliseconds) expectNoEvents() - advanceTimeBy(threeMinutes) + advanceTimeBy(threeMinutes.milliseconds) assertEquals(expectedTwo, awaitItem()) - advanceTimeBy(threeMinutes) + advanceTimeBy(threeMinutes.milliseconds) cancelAndIgnoreRemainingEvents() }