From c672bff18cf40fa613d6f57ad6a5ff15e0438e55 Mon Sep 17 00:00:00 2001 From: Patrick Honkonen <1883101+SaintPatrck@users.noreply.github.com> Date: Wed, 5 Feb 2025 08:46:52 -0500 Subject: [PATCH] [PM-17694] Only update FIDO2 user verification status during single-tap sign-in (#4680) --- .../java/com/x8bit/bitwarden/MainViewModel.kt | 13 ++- .../model/Fido2CredentialAssertionRequest.kt | 13 +++ .../autofill/fido2/util/Fido2IntentUtils.kt | 3 +- .../fido2/util/Fido2IntentUtilsTest.kt | 109 ++++++++++++++++-- 4 files changed, 123 insertions(+), 15 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/MainViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/MainViewModel.kt index 54ad572c6a..5c47d229b3 100644 --- a/app/src/main/java/com/x8bit/bitwarden/MainViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/MainViewModel.kt @@ -339,9 +339,8 @@ class MainViewModel @Inject constructor( // Set the user's verification status when a new FIDO 2 request is received to force // explicit verification if the user's vault is unlocked when the request is // received. - fido2CredentialManager.isUserVerified = - fido2CreateCredentialRequestData.isUserVerified - ?: fido2CredentialManager.isUserVerified + fido2CreateCredentialRequestData.isUserVerified + ?.let { isVerified -> fido2CredentialManager.isUserVerified = isVerified } specialCircumstanceManager.specialCircumstance = SpecialCircumstance.Fido2Save( fido2CreateCredentialRequest = fido2CreateCredentialRequestData, @@ -356,9 +355,11 @@ class MainViewModel @Inject constructor( } fido2CredentialAssertionRequest != null -> { - fido2CredentialManager.isUserVerified = - fido2CredentialAssertionRequest.isUserVerified - ?: false + // If device biometric verification was performed as part of single-tap + // authentication, set the user's verification state to the device result. + // Otherwise, retain the verification state as-is. + fido2CredentialAssertionRequest.isUserVerified + ?.let { isVerified -> fido2CredentialManager.isUserVerified = isVerified } specialCircumstanceManager.specialCircumstance = SpecialCircumstance.Fido2Assertion( fido2AssertionRequest = fido2CredentialAssertionRequest, diff --git a/app/src/main/java/com/x8bit/bitwarden/data/autofill/fido2/model/Fido2CredentialAssertionRequest.kt b/app/src/main/java/com/x8bit/bitwarden/data/autofill/fido2/model/Fido2CredentialAssertionRequest.kt index 1f9b8a5d1f..293e41759c 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/autofill/fido2/model/Fido2CredentialAssertionRequest.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/autofill/fido2/model/Fido2CredentialAssertionRequest.kt @@ -7,6 +7,19 @@ import kotlinx.parcelize.Parcelize /** * Models a FIDO 2 credential authentication request parsed from the launching intent. + * + * @param userId The ID of the Bitwarden user to authenticate. + * @param cipherId The ID of the cipher that contains the passkey to authenticate. + * @param credentialId The ID of the credential to authenticate. + * @param requestJson The JSON representation of the FIDO 2 request. + * @param clientDataHash The hash of the client data. + * @param packageName The package name of the calling app. + * @param signingInfo The signing info of the calling app. + * @param origin The origin of the calling app. Only populated if the calling application is a + * privileged application. I.e., a web browser. + * @param isUserVerified Whether the user has been verified prior to receiving this request. Only + * populated if device biometric verification was performed. If null, the application is responsible + * for prompting user verification when it is deemed necessary. */ @Parcelize data class Fido2CredentialAssertionRequest( diff --git a/app/src/main/java/com/x8bit/bitwarden/data/autofill/fido2/util/Fido2IntentUtils.kt b/app/src/main/java/com/x8bit/bitwarden/data/autofill/fido2/util/Fido2IntentUtils.kt index 3549e46e60..1ec6d30bcc 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/autofill/fido2/util/Fido2IntentUtils.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/autofill/fido2/util/Fido2IntentUtils.kt @@ -39,7 +39,7 @@ fun Intent.getFido2CreateCredentialRequestOrNull(): Fido2CreateCredentialRequest packageName = systemRequest.callingAppInfo.packageName, signingInfo = systemRequest.callingAppInfo.signingInfo, origin = systemRequest.callingAppInfo.origin, - isUserVerified = systemRequest.biometricPromptResult?.isSuccessful ?: false, + isUserVerified = systemRequest.biometricPromptResult?.isSuccessful, ) } @@ -69,7 +69,6 @@ fun Intent.getFido2AssertionRequestOrNull(): Fido2CredentialAssertionRequest? { ?: return null val isUserVerified = systemRequest.biometricPromptResult?.isSuccessful - ?: false return Fido2CredentialAssertionRequest( userId = userId, diff --git a/app/src/test/java/com/x8bit/bitwarden/data/autofill/fido2/util/Fido2IntentUtilsTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/autofill/fido2/util/Fido2IntentUtilsTest.kt index 49def471da..2ece56aa47 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/autofill/fido2/util/Fido2IntentUtilsTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/autofill/fido2/util/Fido2IntentUtilsTest.kt @@ -49,8 +49,9 @@ class Fido2IntentUtilsTest { unmockkObject(PendingIntentHandler.Companion) } + @Suppress("MaxLineLength") @Test - fun `getFido2CredentialRequestOrNull should return Fido2CredentialRequest when present`() { + fun `getFido2CreateCredentialRequestOrNull should return Fido2CreateCredentialRequest when present`() { val intent = mockk { every { getStringExtra(EXTRA_KEY_USER_ID) } returns "mockUserId" } @@ -83,14 +84,59 @@ class Fido2IntentUtilsTest { packageName = mockCallingAppInfo.packageName, signingInfo = mockCallingAppInfo.signingInfo, origin = mockCallingAppInfo.origin, - isUserVerified = false, + isUserVerified = null, ), createRequest, ) } + @Suppress("MaxLineLength") @Test - fun `getFido2CredentialRequestOrNull should return null when build version is below 34`() { + fun `getFido2CreateCredentialRequestOrNull should set isUserVerified when biometric prompt result is present`() { + val intent = mockk { + every { getStringExtra(EXTRA_KEY_USER_ID) } returns "mockUserId" + } + val mockCallingRequest = mockk { + every { requestJson } returns "requestJson" + every { clientDataHash } returns byteArrayOf(0) + every { preferImmediatelyAvailableCredentials } returns false + every { origin } returns "mockOrigin" + every { isAutoSelectAllowed } returns true + } + val mockCallingAppInfo = CallingAppInfo( + packageName = "mockPackageName", + signingInfo = SigningInfo(), + origin = "mockOrigin", + ) + val mockProviderRequest = ProviderCreateCredentialRequest( + callingRequest = mockCallingRequest, + callingAppInfo = mockCallingAppInfo, + biometricPromptResult = mockk { + every { isSuccessful } returns true + }, + ) + + every { + PendingIntentHandler.retrieveProviderCreateCredentialRequest(intent) + } returns mockProviderRequest + + val createRequest = intent.getFido2CreateCredentialRequestOrNull() + assertEquals( + Fido2CreateCredentialRequest( + userId = "mockUserId", + requestJson = mockCallingRequest.requestJson, + packageName = mockCallingAppInfo.packageName, + signingInfo = mockCallingAppInfo.signingInfo, + origin = mockCallingAppInfo.origin, + isUserVerified = true, + ), + createRequest, + ) + } + + @Suppress("MaxLineLength") + @Test + fun `getFido2CreateCredentialRequestOrNull should return null when build version is below 34`() { val intent = mockk() every { isBuildVersionBelow(34) } returns true @@ -100,7 +146,7 @@ class Fido2IntentUtilsTest { @Suppress("MaxLineLength") @Test - fun `getFido2CredentialRequestOrNull should return null when intent is not a provider create credential request`() { + fun `getFido2CreateCredentialRequestOrNull should return null when intent is not a provider create credential request`() { val intent = mockk() every { @@ -112,7 +158,7 @@ class Fido2IntentUtilsTest { @Suppress("MaxLineLength") @Test - fun `getFido2CredentialRequestOrNull should return null when calling request is not a public key credential create request`() { + fun `getFido2CreateCredentialRequestOrNull should return null when calling request is not a public key credential create request`() { val intent = mockk() val mockCallingRequest = mockk() val mockCallingAppInfo = CallingAppInfo( @@ -133,7 +179,7 @@ class Fido2IntentUtilsTest { @Suppress("MaxLineLength") @Test - fun `getFido2CredentialRequestOrNull should return null when user id is not present in extras`() { + fun `getFido2CreateCredentialRequestOrNull should return null when user id is not present in extras`() { val intent = mockk { every { getStringExtra(EXTRA_KEY_USER_ID) } returns null } @@ -200,7 +246,56 @@ class Fido2IntentUtilsTest { packageName = mockCallingAppInfo.packageName, signingInfo = mockCallingAppInfo.signingInfo, origin = mockCallingAppInfo.origin, - isUserVerified = false, + isUserVerified = null, + ), + assertionRequest, + ) + } + + @Suppress("MaxLineLength") + @Test + fun `getFido2AssertionRequestOrNull should set isUserVerified when biometric prompt result is present`() { + val intent = mockk { + every { getStringExtra(EXTRA_KEY_USER_ID) } returns "mockUserId" + every { getStringExtra(EXTRA_KEY_CIPHER_ID) } returns "mockCipherId" + every { getStringExtra(EXTRA_KEY_CREDENTIAL_ID) } returns "mockCredentialId" + } + val mockOption = GetPublicKeyCredentialOption( + requestJson = "requestJson", + clientDataHash = byteArrayOf(0), + allowedProviders = emptySet(), + ) + val mockCallingAppInfo = CallingAppInfo( + packageName = "mockPackageName", + signingInfo = SigningInfo(), + origin = "mockOrigin", + ) + val mockProviderGetCredentialRequest = ProviderGetCredentialRequest( + credentialOptions = listOf(mockOption), + callingAppInfo = mockCallingAppInfo, + biometricPromptResult = mockk { + every { isSuccessful } returns true + }, + ) + + every { + PendingIntentHandler.retrieveProviderGetCredentialRequest(intent) + } returns mockProviderGetCredentialRequest + + val assertionRequest = intent.getFido2AssertionRequestOrNull() + + assertNotNull(assertionRequest) + assertEquals( + Fido2CredentialAssertionRequest( + userId = "mockUserId", + cipherId = "mockCipherId", + credentialId = "mockCredentialId", + requestJson = mockOption.requestJson, + clientDataHash = mockOption.clientDataHash, + packageName = mockCallingAppInfo.packageName, + signingInfo = mockCallingAppInfo.signingInfo, + origin = mockCallingAppInfo.origin, + isUserVerified = true, ), assertionRequest, )