From cdf0f30c61e55fc94524c2fd3f07ffda367555f1 Mon Sep 17 00:00:00 2001 From: Harsh Shandilya Date: Fri, 21 Oct 2022 21:36:27 +0530 Subject: [PATCH] Refactor `format-common` module (#2196) * fix: touch up `PasswordEntryTest` KDoc * feat: add format-common-impl module * refactor: switch app to format-common-impl * refactor: move `format-common` tests to `format-common-impl` * feat: add a test for Steam OTP --- app/build.gradle.kts | 2 +- detekt-baselines/app.xml | 3 -- detekt-baselines/format-common-impl.xml | 7 ++++ format-common-impl/build.gradle.kts | 17 ++++++++++ .../passwordstore/util/totp/UriTotpFinder.kt | 13 ++++--- .../data/passfile/PasswordEntryTest.kt | 34 +++++-------------- .../passwordstore/util/time/TestUserClock.kt | 0 .../app/passwordstore/util/totp/OtpTest.kt | 22 +++++++++++- format-common/build.gradle.kts | 6 +--- .../data/passfile/PasswordEntry.kt | 9 +++-- .../kotlin/app/passwordstore/util/totp/Otp.kt | 7 ++-- settings.gradle.kts | 2 ++ 12 files changed, 73 insertions(+), 49 deletions(-) create mode 100644 detekt-baselines/format-common-impl.xml create mode 100644 format-common-impl/build.gradle.kts rename {app/src/main/java => format-common-impl/src/main/kotlin}/app/passwordstore/util/totp/UriTotpFinder.kt (87%) rename {format-common => format-common-impl}/src/test/kotlin/app/passwordstore/data/passfile/PasswordEntryTest.kt (90%) rename {format-common => format-common-impl}/src/test/kotlin/app/passwordstore/util/time/TestUserClock.kt (100%) rename {format-common => format-common-impl}/src/test/kotlin/app/passwordstore/util/totp/OtpTest.kt (85%) diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 32291d33..54bf2af7 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -56,7 +56,7 @@ dependencies { implementation(projects.autofillParser) implementation(projects.coroutineUtils) implementation(projects.cryptoPgpainless) - implementation(projects.formatCommon) + implementation(projects.formatCommonImpl) implementation(projects.passgen.diceware) implementation(projects.passgen.random) implementation(projects.uiCompose) diff --git a/detekt-baselines/app.xml b/detekt-baselines/app.xml index 5a5f9df9..4ef4ba76 100644 --- a/detekt-baselines/app.xml +++ b/detekt-baselines/app.xml @@ -14,7 +14,6 @@ ComplexMethod:PasswordCreationActivity.kt$PasswordCreationActivity$override fun onCreate(savedInstanceState: Bundle?) ComplexMethod:PasswordCreationActivity.kt$PasswordCreationActivity$private fun encrypt() ComplexMethod:PasswordFragment.kt$PasswordFragment$private fun initializePasswordList() - EmptyDoWhileBlock:PasswordDialog.kt$PasswordDialog${} EmptyFunctionBlock:BasicBottomSheet.kt$BasicBottomSheet.<no name provided>${} EmptyFunctionBlock:ItemCreationBottomSheet.kt$ItemCreationBottomSheet.<no name provided>${} EmptyFunctionBlock:PasswordFragment.kt$PasswordFragment.<no name provided>.<no name provided>${} @@ -58,7 +57,6 @@ MagicNumber:SshKey.kt$SshKey.Algorithm.Ecdsa$256 MagicNumber:SshKey.kt$SshKey.Algorithm.Rsa$3072 MagicNumber:SshjSessionFactory.kt$SshjSession$22 - MagicNumber:UriTotpFinder.kt$UriTotpFinder$30 MatchingDeclarationName:AutofillViewUtils.kt$DatasetMetadata MaxLineLength:BaseGitActivity.kt$BaseGitActivity$"The server does not support multiple Git operations per SSH session. Please try again, a slower fallback mode will be used." MaxLineLength:BaseGitActivity.kt$BaseGitActivity$"WARNING: The remote host key has changed. If this is expected, please go to Git server settings and clear the saved host key." @@ -82,7 +80,6 @@ ReturnCount:OreoAutofillService.kt$OreoAutofillService$override fun onSaveRequest(request: SaveRequest, callback: SaveCallback) ReturnCount:PasswordStore.kt$PasswordStore$override fun onKeyDown(keyCode: Int, event: KeyEvent): Boolean ReturnCount:ShortcutHandler.kt$ShortcutHandler$fun addPinnedShortcut(item: PasswordItem, intent: Intent) - ReturnCount:UriTotpFinder.kt$UriTotpFinder$override fun findSecret(content: String): String? SpreadOperator:Api30AutofillResponseBuilder.kt$Api30AutofillResponseBuilder$(*ignoredIds.toTypedArray()) SpreadOperator:AutofillResponseBuilder.kt$AutofillResponseBuilder$(*ignoredIds.toTypedArray()) SpreadOperator:BreakOutOfDetached.kt$BreakOutOfDetached$( // abort the rebase git.rebase().setOperation(RebaseCommand.Operation.ABORT), *resetCommands, ) diff --git a/detekt-baselines/format-common-impl.xml b/detekt-baselines/format-common-impl.xml new file mode 100644 index 00000000..5a23ddfe --- /dev/null +++ b/detekt-baselines/format-common-impl.xml @@ -0,0 +1,7 @@ + + + + + ReturnCount:UriTotpFinder.kt$UriTotpFinder$override fun findSecret(content: String): String? + + diff --git a/format-common-impl/build.gradle.kts b/format-common-impl/build.gradle.kts new file mode 100644 index 00000000..c3092f6d --- /dev/null +++ b/format-common-impl/build.gradle.kts @@ -0,0 +1,17 @@ +plugins { + id("com.github.android-password-store.android-library") + id("com.github.android-password-store.kotlin-android") + id("com.github.android-password-store.kotlin-library") +} + +android { namespace = "app.passwordstore.format.common.impl" } + +dependencies { + api(projects.formatCommon) + implementation(libs.dagger.hilt.core) + testImplementation(projects.coroutineUtilsTesting) + testImplementation(libs.bundles.testDependencies) + testImplementation(libs.kotlin.coroutines.test) + testImplementation(libs.testing.robolectric) + testImplementation(libs.testing.turbine) +} diff --git a/app/src/main/java/app/passwordstore/util/totp/UriTotpFinder.kt b/format-common-impl/src/main/kotlin/app/passwordstore/util/totp/UriTotpFinder.kt similarity index 87% rename from app/src/main/java/app/passwordstore/util/totp/UriTotpFinder.kt rename to format-common-impl/src/main/kotlin/app/passwordstore/util/totp/UriTotpFinder.kt index 30447690..741a21a7 100644 --- a/app/src/main/java/app/passwordstore/util/totp/UriTotpFinder.kt +++ b/format-common-impl/src/main/kotlin/app/passwordstore/util/totp/UriTotpFinder.kt @@ -1,15 +1,14 @@ -/* - * Copyright © 2014-2021 The Android Password Store Authors. All Rights Reserved. - * SPDX-License-Identifier: GPL-3.0-only - */ - package app.passwordstore.util.totp import android.net.Uri import javax.inject.Inject /** [Uri] backed TOTP URL parser. */ -class UriTotpFinder @Inject constructor() : TotpFinder { +public class UriTotpFinder @Inject constructor() : TotpFinder { + + private companion object { + private const val DEFAULT_TOTP_PERIOD = 30L + } override fun findSecret(content: String): String? { content.split("\n".toRegex()).forEach { line -> @@ -28,7 +27,7 @@ class UriTotpFinder @Inject constructor() : TotpFinder { } override fun findPeriod(content: String): Long { - return getQueryParameter(content, "period")?.toLongOrNull() ?: 30 + return getQueryParameter(content, "period")?.toLongOrNull() ?: DEFAULT_TOTP_PERIOD } override fun findAlgorithm(content: String): String { diff --git a/format-common/src/test/kotlin/app/passwordstore/data/passfile/PasswordEntryTest.kt b/format-common-impl/src/test/kotlin/app/passwordstore/data/passfile/PasswordEntryTest.kt similarity index 90% rename from format-common/src/test/kotlin/app/passwordstore/data/passfile/PasswordEntryTest.kt rename to format-common-impl/src/test/kotlin/app/passwordstore/data/passfile/PasswordEntryTest.kt index 2022d968..1fa50188 100644 --- a/format-common/src/test/kotlin/app/passwordstore/data/passfile/PasswordEntryTest.kt +++ b/format-common-impl/src/test/kotlin/app/passwordstore/data/passfile/PasswordEntryTest.kt @@ -9,7 +9,7 @@ import app.cash.turbine.test import app.passwordstore.test.CoroutineTestRule import app.passwordstore.util.time.TestUserClock import app.passwordstore.util.time.UserClock -import app.passwordstore.util.totp.TotpFinder +import app.passwordstore.util.totp.UriTotpFinder import java.util.Locale import kotlin.test.Test import kotlin.test.assertEquals @@ -21,15 +21,20 @@ import kotlin.time.ExperimentalTime import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.runTest import org.junit.Rule +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner @OptIn(ExperimentalCoroutinesApi::class, ExperimentalTime::class) +@RunWith(RobolectricTestRunner::class) class PasswordEntryTest { @get:Rule val coroutineTestRule: CoroutineTestRule = CoroutineTestRule() + private val totpFinder = UriTotpFinder() + private fun makeEntry(content: String, clock: UserClock = fakeClock) = PasswordEntry( clock, - testFinder, + totpFinder, content.encodeToByteArray(), ) @@ -144,7 +149,7 @@ class PasswordEntryTest { } /** - * Same as [testGeneratesOtpFromTotpUri], but advances the clock by 5 seconds. This exercises the + * Same as [generatesOtpFromTotpUri], but advances the clock by 5 seconds. This exercises the * [Totp.remainingTime] calculation logic, and acts as a regression test to resolve the bug which * blocked https://msfjarvis.dev/aps/issue/1550. */ @@ -199,28 +204,5 @@ class PasswordEntryTest { "otpauth://totp/ACME%20Co:john@example.com?secret=HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ&issuer=ACME%20Co&algorithm=SHA1&digits=6&period=30" val fakeClock = TestUserClock() - - // This implementation is hardcoded for the URI above. - val testFinder = - object : TotpFinder { - override fun findSecret(content: String): String { - return "HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ" - } - - override fun findDigits(content: String): String { - return "6" - } - - override fun findPeriod(content: String): Long { - return 30 - } - - override fun findAlgorithm(content: String): String { - return "SHA1" - } - override fun findIssuer(content: String): String { - return "ACME Co" - } - } } } diff --git a/format-common/src/test/kotlin/app/passwordstore/util/time/TestUserClock.kt b/format-common-impl/src/test/kotlin/app/passwordstore/util/time/TestUserClock.kt similarity index 100% rename from format-common/src/test/kotlin/app/passwordstore/util/time/TestUserClock.kt rename to format-common-impl/src/test/kotlin/app/passwordstore/util/time/TestUserClock.kt diff --git a/format-common/src/test/kotlin/app/passwordstore/util/totp/OtpTest.kt b/format-common-impl/src/test/kotlin/app/passwordstore/util/totp/OtpTest.kt similarity index 85% rename from format-common/src/test/kotlin/app/passwordstore/util/totp/OtpTest.kt rename to format-common-impl/src/test/kotlin/app/passwordstore/util/totp/OtpTest.kt index bf8cd186..54ac492d 100644 --- a/format-common/src/test/kotlin/app/passwordstore/util/totp/OtpTest.kt +++ b/format-common-impl/src/test/kotlin/app/passwordstore/util/totp/OtpTest.kt @@ -137,7 +137,7 @@ class OtpTest { ) val paddedOtp = generateOtp( - 1593367171420 / (1000 * 30), + counter = 1593367171420 / (1000 * 30), secret = "ON2HE2LOM4QHO2LUNAQHG33NMUQHAYLEMRUW4ZZANZSWKZDFMQFA====", ) @@ -145,4 +145,24 @@ class OtpTest { assertNotNull(paddedOtp) assertEquals(unpaddedOtp, paddedOtp) } + + @Test + fun generateSteamTotp() { + val issuerOtp = + generateOtp( + counter = 48297900 / (1000 * 30), + secret = "STK7746GVMCHMNH5FBIAQXGPV3I7ZHRG", + issuer = "Steam", + ) + val digitsOtp = + generateOtp( + counter = 48297900 / (1000 * 30), + secret = "STK7746GVMCHMNH5FBIAQXGPV3I7ZHRG", + digits = "s", + ) + assertNotNull(issuerOtp) + assertNotNull(digitsOtp) + assertEquals("6M3CT", issuerOtp) + assertEquals("6M3CT", digitsOtp) + } } diff --git a/format-common/build.gradle.kts b/format-common/build.gradle.kts index f18dc906..722f5259 100644 --- a/format-common/build.gradle.kts +++ b/format-common/build.gradle.kts @@ -9,14 +9,10 @@ plugins { } dependencies { + api(libs.thirdparty.kotlinResult) implementation(projects.coroutineUtils) implementation(libs.androidx.annotation) implementation(libs.dagger.hilt.core) implementation(libs.thirdparty.commons.codec) - implementation(libs.thirdparty.kotlinResult) implementation(libs.kotlin.coroutines.core) - testImplementation(projects.coroutineUtilsTesting) - testImplementation(libs.bundles.testDependencies) - testImplementation(libs.kotlin.coroutines.test) - testImplementation(libs.testing.turbine) } diff --git a/format-common/src/main/kotlin/app/passwordstore/data/passfile/PasswordEntry.kt b/format-common/src/main/kotlin/app/passwordstore/data/passfile/PasswordEntry.kt index 38aa4a20..d48c2ca3 100644 --- a/format-common/src/main/kotlin/app/passwordstore/data/passfile/PasswordEntry.kt +++ b/format-common/src/main/kotlin/app/passwordstore/data/passfile/PasswordEntry.kt @@ -5,6 +5,7 @@ package app.passwordstore.data.passfile +import androidx.annotation.VisibleForTesting import app.passwordstore.util.time.UserClock import app.passwordstore.util.totp.Otp import app.passwordstore.util.totp.TotpFinder @@ -202,10 +203,11 @@ constructor( public fun create(bytes: ByteArray): PasswordEntry } - internal companion object { + public companion object { private const val EXTRA_CONTENT = "Extra Content" - internal val USERNAME_FIELDS = + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + public val USERNAME_FIELDS: Array = arrayOf( "login:", "username:", @@ -218,7 +220,8 @@ constructor( "id:", "identity:", ) - internal val PASSWORD_FIELDS = + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + public val PASSWORD_FIELDS: Array = arrayOf( "password:", "secret:", diff --git a/format-common/src/main/kotlin/app/passwordstore/util/totp/Otp.kt b/format-common/src/main/kotlin/app/passwordstore/util/totp/Otp.kt index 5abc0337..10e771fe 100644 --- a/format-common/src/main/kotlin/app/passwordstore/util/totp/Otp.kt +++ b/format-common/src/main/kotlin/app/passwordstore/util/totp/Otp.kt @@ -6,6 +6,7 @@ package app.passwordstore.util.totp import com.github.michaelbull.result.Err +import com.github.michaelbull.result.Result import com.github.michaelbull.result.runCatching import java.nio.ByteBuffer import java.util.Locale @@ -14,7 +15,7 @@ import javax.crypto.spec.SecretKeySpec import kotlin.experimental.and import org.apache.commons.codec.binary.Base32 -internal object Otp { +public object Otp { private val BASE_32 = Base32() private val STEAM_ALPHABET = "23456789BCDFGHJKMNPQRTVWXY".toCharArray() @@ -26,13 +27,13 @@ internal object Otp { private const val ALPHABET_LENGTH = 26 private const val MOST_SIGNIFICANT_BYTE = 0x7f - fun calculateCode( + public fun calculateCode( secret: String, counter: Long, algorithm: String, digits: String, issuer: String?, - ) = runCatching { + ): Result = runCatching { val algo = "Hmac${algorithm.uppercase(Locale.ROOT)}" val decodedSecret = BASE_32.decode(secret) val secretKey = SecretKeySpec(decodedSecret, algo) diff --git a/settings.gradle.kts b/settings.gradle.kts index df15a3a0..6e6ce14f 100644 --- a/settings.gradle.kts +++ b/settings.gradle.kts @@ -178,6 +178,8 @@ include("crypto-pgpainless") include("format-common") +include("format-common-impl") + include("passgen:diceware") include("passgen:random")