Refactor PasswordEntry TOTP calculation into a cold flow (#1702)

This commit is contained in:
Harsh Shandilya 2022-02-01 19:21:01 +05:30 committed by GitHub
parent cf92d8a5a3
commit cf111f1978
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 103 additions and 95 deletions

View file

@ -43,7 +43,6 @@ import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import logcat.LogPriority.ERROR
import logcat.asLog
import logcat.logcat
import me.msfjarvis.openpgpktx.util.OpenPgpApi
import me.msfjarvis.openpgpktx.util.OpenPgpServiceConnection
@ -204,7 +203,7 @@ class AutofillDecryptActivity : AppCompatActivity() {
val entry =
withContext(Dispatchers.IO) {
@Suppress("BlockingMethodInNonBlockingContext")
passwordEntryFactory.create(lifecycleScope, decryptedOutput.toByteArray())
passwordEntryFactory.create(decryptedOutput.toByteArray())
}
AutofillPreferences.credentialsFromStoreEntry(
this,

View file

@ -162,7 +162,7 @@ class AutofillDecryptActivityV2 : AppCompatActivity() {
}
.onSuccess { result ->
return runCatching {
val entry = passwordEntryFactory.create(lifecycleScope, result.toByteArray())
val entry = passwordEntryFactory.create(result.toByteArray())
AutofillPreferences.credentialsFromStoreEntry(this, file, entry, directoryStructure)
}
.getOrElse { e ->

View file

@ -31,6 +31,7 @@ import kotlin.time.ExperimentalTime
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.collect
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
@ -178,7 +179,7 @@ class DecryptActivity : BasePgpActivity(), OpenPgpServiceConnection.OnBound {
startAutoDismissTimer()
runCatching {
val showPassword = settings.getBoolean(PreferenceKeys.SHOW_PASSWORD, true)
val entry = passwordEntryFactory.create(lifecycleScope, outputStream.toByteArray())
val entry = passwordEntryFactory.create(outputStream.toByteArray())
if (settings.getBoolean(PreferenceKeys.COPY_ON_DECRYPT, false)) {
copyPasswordToClipboard(entry.password)
@ -193,7 +194,7 @@ class DecryptActivity : BasePgpActivity(), OpenPgpServiceConnection.OnBound {
}
if (entry.hasTotp()) {
items.add(FieldItem.createOtpField(entry.totp.value))
items.add(FieldItem.createOtpField(entry.totp.first()))
}
if (!entry.username.isNullOrBlank()) {

View file

@ -31,6 +31,7 @@ import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.collect
import kotlinx.coroutines.flow.collectLatest
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
@ -166,7 +167,7 @@ class DecryptActivityV2 : BasePgpActivity() {
require(result.size() != 0) { "Incorrect password" }
startAutoDismissTimer()
val entry = passwordEntryFactory.create(lifecycleScope, result.toByteArray())
val entry = passwordEntryFactory.create(result.toByteArray())
passwordEntry = entry
createPasswordUi(entry)
}
@ -182,7 +183,7 @@ class DecryptActivityV2 : BasePgpActivity() {
}
if (entry.hasTotp()) {
items.add(FieldItem.createOtpField(entry.totp.value))
items.add(FieldItem.createOtpField(entry.totp.first()))
}
if (!entry.username.isNullOrBlank()) {

View file

@ -293,10 +293,7 @@ class PasswordCreationActivity : BasePgpActivity(), OpenPgpServiceConnection.OnB
// User wants to disable username encryption, so we extract the
// username from the encrypted extras and use it as the filename.
val entry =
passwordEntryFactory.create(
lifecycleScope,
"PASSWORD\n${extraContent.text}".encodeToByteArray()
)
passwordEntryFactory.create("PASSWORD\n${extraContent.text}".encodeToByteArray())
val username = entry.username
// username should not be null here by the logic in
@ -370,10 +367,7 @@ class PasswordCreationActivity : BasePgpActivity(), OpenPgpServiceConnection.OnB
with(binding) {
// Use PasswordEntry to parse extras for username
val entry =
passwordEntryFactory.create(
lifecycleScope,
"PLACEHOLDER\n${extraContent.text}".encodeToByteArray()
)
passwordEntryFactory.create("PLACEHOLDER\n${extraContent.text}".encodeToByteArray())
encryptUsername.apply {
if (visibility != View.VISIBLE) return@apply
val hasUsernameInFileName = filename.text.toString().isNotBlank()
@ -529,7 +523,7 @@ class PasswordCreationActivity : BasePgpActivity(), OpenPgpServiceConnection.OnB
if (shouldGeneratePassword) {
val directoryStructure = AutofillPreferences.directoryStructure(applicationContext)
val entry = passwordEntryFactory.create(lifecycleScope, content.encodeToByteArray())
val entry = passwordEntryFactory.create(content.encodeToByteArray())
returnIntent.putExtra(RETURN_EXTRA_PASSWORD, entry.password)
val username = entry.username ?: directoryStructure.getUsernameFor(file)
returnIntent.putExtra(RETURN_EXTRA_USERNAME, username)

View file

@ -223,10 +223,7 @@ class PasswordCreationActivityV2 : BasePgpActivity() {
// User wants to disable username encryption, so we extract the
// username from the encrypted extras and use it as the filename.
val entry =
passwordEntryFactory.create(
lifecycleScope,
"PASSWORD\n${extraContent.text}".encodeToByteArray()
)
passwordEntryFactory.create("PASSWORD\n${extraContent.text}".encodeToByteArray())
val username = entry.username
// username should not be null here by the logic in
@ -300,10 +297,7 @@ class PasswordCreationActivityV2 : BasePgpActivity() {
with(binding) {
// Use PasswordEntry to parse extras for username
val entry =
passwordEntryFactory.create(
lifecycleScope,
"PLACEHOLDER\n${extraContent.text}".encodeToByteArray()
)
passwordEntryFactory.create("PLACEHOLDER\n${extraContent.text}".encodeToByteArray())
encryptUsername.apply {
if (visibility != View.VISIBLE) return@apply
val hasUsernameInFileName = filename.text.toString().isNotBlank()
@ -406,7 +400,7 @@ class PasswordCreationActivityV2 : BasePgpActivity() {
if (shouldGeneratePassword) {
val directoryStructure = AutofillPreferences.directoryStructure(applicationContext)
val entry = passwordEntryFactory.create(lifecycleScope, content.encodeToByteArray())
val entry = passwordEntryFactory.create(content.encodeToByteArray())
returnIntent.putExtra(RETURN_EXTRA_PASSWORD, entry.password)
val username = entry.username ?: directoryStructure.getUsernameFor(file)
returnIntent.putExtra(RETURN_EXTRA_USERNAME, username)

View file

@ -15,6 +15,8 @@ import dev.msfjarvis.aps.util.services.getDefaultUsername
import dev.msfjarvis.aps.util.settings.PreferenceKeys
import java.io.File
import java.nio.file.Paths
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.runBlocking
enum class DirectoryStructure(val value: String) {
EncryptedUsername("encrypted_username"),
@ -141,6 +143,6 @@ object AutofillPreferences {
// Always give priority to a username stored in the encrypted extras
val username =
entry.username ?: directoryStructure.getUsernameFor(file) ?: context.getDefaultUsername()
return Credentials(username, entry.password, entry.totp.value)
return Credentials(username, entry.password, runBlocking { entry.totp.first() })
}
}

View file

@ -11,4 +11,5 @@ dependencies {
implementation(projects.coroutineUtils)
implementation(libs.testing.junit)
implementation(libs.kotlin.coroutines.test)
api(libs.testing.turbine)
}

View file

@ -0,0 +1,37 @@
/*
* Copyright © 2014-2022 The Android Password Store Authors. All Rights Reserved.
* SPDX-License-Identifier: GPL-3.0-only
*/
package dev.msfjarvis.aps.test
import app.cash.turbine.FlowTurbine
import app.cash.turbine.test
import kotlin.coroutines.coroutineContext
import kotlin.time.Duration
import kotlin.time.Duration.Companion.seconds
import kotlin.time.ExperimentalTime
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flowOn
import kotlinx.coroutines.test.TestCoroutineScheduler
import kotlinx.coroutines.test.UnconfinedTestDispatcher
/**
* Wrapper for [test] that implements compatibility with kotlinx.coroutines 1.6.0
*
* @see "https://github.com/cashapp/turbine/issues/42#issuecomment-1000317026"
*/
@ExperimentalTime
@ExperimentalCoroutinesApi
public suspend fun <T> Flow<T>.test2(
timeout: Duration = 1.seconds,
validate: suspend FlowTurbine<T>.() -> Unit,
) {
val testScheduler = coroutineContext[TestCoroutineScheduler]
return if (testScheduler == null) {
test(timeout, validate)
} else {
flowOn(UnconfinedTestDispatcher(testScheduler)).test(timeout, validate)
}
}

View file

@ -9,20 +9,15 @@ import com.github.michaelbull.result.mapBoth
import dagger.assisted.Assisted
import dagger.assisted.AssistedFactory
import dagger.assisted.AssistedInject
import dev.msfjarvis.aps.util.coroutines.DispatcherProvider
import dev.msfjarvis.aps.util.time.UserClock
import dev.msfjarvis.aps.util.totp.Otp
import dev.msfjarvis.aps.util.totp.TotpFinder
import kotlin.collections.set
import kotlin.time.Duration
import kotlin.time.ExperimentalTime
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.awaitCancellation
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.asStateFlow
import kotlinx.coroutines.flow.update
import kotlinx.coroutines.launch
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flow
/** Represents a single entry in the password store. */
@OptIn(ExperimentalTime::class)
@ -30,20 +25,13 @@ public class PasswordEntry
@AssistedInject
constructor(
/** A time source used to calculate the TOTP */
clock: UserClock,
private val clock: UserClock,
/** [TotpFinder] implementation to extract data from a TOTP URI */
totpFinder: TotpFinder,
/** Instance of [DispatcherProvider] to select an IO dispatcher for emitting TOTP values. */
dispatcherProvider: DispatcherProvider,
/**
* A cancellable [CoroutineScope] inside which we constantly emit new TOTP values as time elapses
*/
@Assisted scope: CoroutineScope,
private val totpFinder: TotpFinder,
/** The content of this entry, as an array of bytes. */
@Assisted bytes: ByteArray,
) {
private val _totp = MutableStateFlow("")
private val content = bytes.decodeToString()
/** The password text for this entry. Can be null. */
@ -62,11 +50,21 @@ constructor(
public val extraContentString: String
/**
* A [StateFlow] providing the current TOTP. It will emit a single empty string on initialization
* which is replaced with a real TOTP if applicable. Call [hasTotp] to verify whether or not you
* need to observe this value.
* A [Flow] providing the current TOTP. It will start emitting only when collected. If this entry
* does not have a TOTP secret, the flow will never emit. Users should call [hasTotp] before
* collection to check if it is valid to collect this [Flow].
*/
public val totp: StateFlow<String> = _totp.asStateFlow()
public val totp: Flow<String> = flow {
if (totpSecret != null) {
repeat(Int.MAX_VALUE) {
val (otp, remainingTime) = calculateTotp()
emit(otp)
delay(remainingTime)
}
} else {
awaitCancellation()
}
}
/**
* String representation of [extraContent] but with authentication related data such as TOTP URIs
@ -83,21 +81,6 @@ constructor(
extraContent = generateExtraContentPairs()
username = findUsername()
totpSecret = totpFinder.findSecret(content)
if (totpSecret != null) {
scope.launch(dispatcherProvider.io()) {
val digits = totpFinder.findDigits(content)
val totpPeriod = totpFinder.findPeriod(content)
val totpAlgorithm = totpFinder.findAlgorithm(content)
val issuer = totpFinder.findIssuer(content)
val remainingTime = totpPeriod - (clock.millis() % totpPeriod)
updateTotp(clock.millis(), totpPeriod, totpAlgorithm, digits, issuer)
delay(Duration.seconds(remainingTime))
repeat(Int.MAX_VALUE) {
updateTotp(clock.millis(), totpPeriod, totpAlgorithm, digits, issuer)
delay(Duration.seconds(totpPeriod))
}
}
}
}
public fun hasTotp(): Boolean {
@ -188,22 +171,25 @@ constructor(
return null
}
private fun updateTotp(
millis: Long,
totpPeriod: Long,
totpAlgorithm: String,
digits: String,
issuer: String?,
) {
if (totpSecret != null) {
Otp.calculateCode(totpSecret, millis / (1000 * totpPeriod), totpAlgorithm, digits, issuer)
.mapBoth({ code -> _totp.update { code } }, { throwable -> throw throwable })
}
private fun calculateTotp(): Pair<String, Long> {
val digits = totpFinder.findDigits(content)
val totpPeriod = totpFinder.findPeriod(content)
val totpAlgorithm = totpFinder.findAlgorithm(content)
val issuer = totpFinder.findIssuer(content)
val millis = clock.millis()
val remainingTime = totpPeriod - (millis % totpPeriod)
Otp.calculateCode(totpSecret!!, millis / (1000 * totpPeriod), totpAlgorithm, digits, issuer)
.mapBoth(
{ code ->
return code to remainingTime
},
{ throwable -> throw throwable }
)
}
@AssistedFactory
public interface Factory {
public fun create(scope: CoroutineScope, bytes: ByteArray): PasswordEntry
public fun create(bytes: ByteArray): PasswordEntry
}
internal companion object {

View file

@ -6,10 +6,10 @@
package dev.msfjarvis.aps.data.passfile
import dev.msfjarvis.aps.test.CoroutineTestRule
import dev.msfjarvis.aps.test.test2
import dev.msfjarvis.aps.util.time.TestUserClock
import dev.msfjarvis.aps.util.totp.TotpFinder
import java.util.Locale
import kotlin.test.Ignore
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertNotNull
@ -17,7 +17,6 @@ import kotlin.test.assertNull
import kotlin.test.assertTrue
import kotlin.time.ExperimentalTime
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.TestScope
import kotlinx.coroutines.test.runTest
import org.junit.Rule
@ -29,8 +28,6 @@ class PasswordEntryTest {
PasswordEntry(
fakeClock,
testFinder,
coroutineTestRule.testDispatcherProvider,
TestScope(coroutineTestRule.testDispatcher),
content.encodeToByteArray(),
)
@ -133,27 +130,22 @@ class PasswordEntryTest {
}
@Test
@Ignore("Timing with runTest seems hard to implement right now")
fun testGeneratesOtpFromTotpUri() {
runTest {
val entry = makeEntry("secret\nextra\n$TOTP_URI")
assertTrue(entry.hasTotp())
val code = entry.totp.value
assertNotNull(code) { "Generated OTP cannot be null" }
assertEquals("818800", code)
fun testGeneratesOtpFromTotpUri() = runTest {
val entry = makeEntry("secret\nextra\n$TOTP_URI")
assertTrue(entry.hasTotp())
entry.totp.test2 {
assertEquals("818800", expectMostRecentItem())
cancelAndIgnoreRemainingEvents()
}
}
@Test
@Ignore("Timing with runTest seems hard to implement right now")
fun testGeneratesOtpWithOnlyUriInFile() {
runTest {
val entry = makeEntry(TOTP_URI)
assertNull(entry.password)
assertTrue(entry.hasTotp())
val code = entry.totp.value
assertNotNull(code) { "Generated OTP cannot be null" }
assertEquals("818800", code)
fun testGeneratesOtpWithOnlyUriInFile() = runTest {
val entry = makeEntry(TOTP_URI)
assertNull(entry.password)
entry.totp.test2 {
assertEquals("818800", expectMostRecentItem())
cancelAndIgnoreRemainingEvents()
}
}

View file

@ -93,6 +93,7 @@ testing-junit = "junit:junit:4.13.2"
testing-kotlintest-junit = { module = "org.jetbrains.kotlin:kotlin-test-junit", version.ref = "kotlin" }
testing-robolectric = "org.robolectric:robolectric:4.7.3"
testing-sharedPrefsMock = "com.github.android-password-store:shared-preferences-fake:2.0.0"
testing-turbine = "app.cash.turbine:turbine:0.7.0"
[bundles]
androidxLifecycle = ["androidx-lifecycle-common", "androidx-lifecycle-livedataKtx", "androidx-lifecycle-viewmodelKtx"]