From 61374bca4ef4b53f6fa048fc592f39879e1d0651 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 5 May 2026 18:24:25 +0200 Subject: [PATCH] Improve detection of configure PIN code. --- .../impl/biometric/BiometricAuthenticator.kt | 8 +++--- .../impl/pin/DefaultPinCodeManager.kt | 5 ++-- .../impl/storage/EncryptedPinCodeStorage.kt | 7 ----- .../storage/PreferencesLockScreenStore.kt | 6 ----- .../impl/DefaultLockScreenServiceTest.kt | 19 ++++++++----- .../biometric/FakeBiometricAuthenticator.kt | 2 +- .../pin/storage/InMemoryLockScreenStore.kt | 9 ------- libraries/cryptography/api/build.gradle.kts | 4 +++ .../cryptography/api/SecretKeyRepository.kt | 7 +++-- libraries/cryptography/impl/build.gradle.kts | 1 + .../impl/KeyStoreSecretKeyRepository.kt | 27 ++++++++++++++++--- libraries/cryptography/test/build.gradle.kts | 1 + .../test/SimpleSecretKeyRepository.kt | 25 ++++++++++++++--- 13 files changed, 77 insertions(+), 44 deletions(-) diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/biometric/BiometricAuthenticator.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/biometric/BiometricAuthenticator.kt index a96c713ff2..d18d9b73b7 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/biometric/BiometricAuthenticator.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/biometric/BiometricAuthenticator.kt @@ -36,13 +36,13 @@ interface BiometricAuthenticator { } val isActive: Boolean - fun setup() + suspend fun setup() suspend fun authenticate(): AuthenticationResult } class NoopBiometricAuthentication : BiometricAuthenticator { override val isActive: Boolean = false - override fun setup() = Unit + override suspend fun setup() = Unit override suspend fun authenticate() = BiometricAuthenticator.AuthenticationResult.Failure() } @@ -58,7 +58,7 @@ class DefaultBiometricAuthentication( private var cryptoObject: CryptoObject? = null - override fun setup() { + override suspend fun setup() { try { val secretKey = ensureKey() val cipher = encryptionDecryptionService.createEncryptionCipher(secretKey) @@ -86,7 +86,7 @@ class DefaultBiometricAuthentication( } @Throws(KeyPermanentlyInvalidatedException::class) - private fun ensureKey() = secretKeyRepository.getOrCreateKey(keyAlias, true).also { + private suspend fun ensureKey() = secretKeyRepository.getOrCreateKey(keyAlias, true).also { encryptionDecryptionService.createEncryptionCipher(it) } } diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/pin/DefaultPinCodeManager.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/pin/DefaultPinCodeManager.kt index 091432044a..9a646461fa 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/pin/DefaultPinCodeManager.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/pin/DefaultPinCodeManager.kt @@ -18,7 +18,7 @@ import io.element.android.libraries.cryptography.api.SecretKeyRepository import kotlinx.coroutines.flow.Flow import java.util.concurrent.CopyOnWriteArrayList -private const val SECRET_KEY_ALIAS = "elementx.SECRET_KEY_ALIAS_PIN_CODE" +internal const val SECRET_KEY_ALIAS = "elementx.SECRET_KEY_ALIAS_PIN_CODE" @ContributesBinding(AppScope::class) @SingleIn(AppScope::class) @@ -38,7 +38,7 @@ class DefaultPinCodeManager( } override fun hasPinCode(): Flow { - return lockScreenStore.hasPinCode() + return secretKeyRepository.hasKey(SECRET_KEY_ALIAS) } override suspend fun getPinCodeSize(): Int { @@ -79,6 +79,7 @@ class DefaultPinCodeManager( override suspend fun deletePinCode() { lockScreenStore.deleteEncryptedPinCode() lockScreenStore.resetCounter() + secretKeyRepository.deleteKey(SECRET_KEY_ALIAS) callbacks.forEach { it.onPinCodeRemoved() } } diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/EncryptedPinCodeStorage.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/EncryptedPinCodeStorage.kt index c4558812de..b41e6a9578 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/EncryptedPinCodeStorage.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/EncryptedPinCodeStorage.kt @@ -8,8 +8,6 @@ package io.element.android.features.lockscreen.impl.storage -import kotlinx.coroutines.flow.Flow - /** * Should be implemented by any class that provides access to the encrypted PIN code. * All methods are suspending in case there are async IO operations involved. @@ -29,9 +27,4 @@ interface EncryptedPinCodeStorage { * Deletes the PIN code from some persistable storage. */ suspend fun deleteEncryptedPinCode() - - /** - * Returns whether the PIN code is stored or not. - */ - fun hasPinCode(): Flow } diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/PreferencesLockScreenStore.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/PreferencesLockScreenStore.kt index 6b99d90592..98435f962d 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/PreferencesLockScreenStore.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/PreferencesLockScreenStore.kt @@ -70,12 +70,6 @@ class PreferencesLockScreenStore( } } - override fun hasPinCode(): Flow { - return dataStore.data.map { preferences -> - preferences[pinCodeKey] != null - } - } - override fun isBiometricUnlockAllowed(): Flow { return dataStore.data.map { preferences -> preferences[biometricUnlockKey] ?: false diff --git a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/DefaultLockScreenServiceTest.kt b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/DefaultLockScreenServiceTest.kt index 9082f20a55..f906d0d6ba 100644 --- a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/DefaultLockScreenServiceTest.kt +++ b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/DefaultLockScreenServiceTest.kt @@ -14,9 +14,12 @@ import io.element.android.features.lockscreen.impl.biometric.BiometricAuthentica import io.element.android.features.lockscreen.impl.biometric.FakeBiometricAuthenticatorManager import io.element.android.features.lockscreen.impl.fixtures.aLockScreenConfig import io.element.android.features.lockscreen.impl.pin.PinCodeManager +import io.element.android.features.lockscreen.impl.pin.SECRET_KEY_ALIAS import io.element.android.features.lockscreen.impl.pin.createDefaultPinCodeManager import io.element.android.features.lockscreen.impl.pin.storage.InMemoryLockScreenStore import io.element.android.features.lockscreen.impl.storage.LockScreenStore +import io.element.android.libraries.cryptography.api.SecretKeyRepository +import io.element.android.libraries.cryptography.test.SimpleSecretKeyRepository import io.element.android.libraries.sessionstorage.api.observer.SessionObserver import io.element.android.libraries.sessionstorage.test.observer.FakeSessionObserver import io.element.android.services.appnavstate.api.AppForegroundStateService @@ -38,18 +41,18 @@ class DefaultLockScreenServiceTest { @Test fun `when the pin is mandatory, isSetupRequired emits true`() = runTest { - val lockScreenStore = InMemoryLockScreenStore() + val secretKeyRepository = SimpleSecretKeyRepository() val sut = createDefaultLockScreenService( lockScreenConfig = aLockScreenConfig(isPinMandatory = true), - lockScreenStore = lockScreenStore, + secretKeyRepository = secretKeyRepository, ) sut.isSetupRequired().test { assertThat(awaitItem()).isTrue() // When the user configures the pin code, the setup is not required anymore - lockScreenStore.saveEncryptedPinCode("encryptedCode") + secretKeyRepository.getOrCreateKey(SECRET_KEY_ALIAS, true) assertThat(awaitItem()).isFalse() // Users deletes the pin code - lockScreenStore.deleteEncryptedPinCode() + secretKeyRepository.deleteKey("elementx.SECRET_KEY_ALIAS_PIN_CODE") assertThat(awaitItem()).isTrue() } } @@ -57,16 +60,16 @@ class DefaultLockScreenServiceTest { @Test fun `when the last session is deleted, the pin code is removed`() = runTest { val sessionObserver = FakeSessionObserver() - val lockScreenStore = InMemoryLockScreenStore() + val secretKeyRepository = SimpleSecretKeyRepository() val sut = createDefaultLockScreenService( lockScreenConfig = aLockScreenConfig(isPinMandatory = true), - lockScreenStore = lockScreenStore, + secretKeyRepository = secretKeyRepository, sessionObserver = sessionObserver, ) sut.isPinSetup().test { assertThat(awaitItem()).isFalse() // When the user configure the pin code, the setup is not required anymore - lockScreenStore.saveEncryptedPinCode("encryptedCode") + secretKeyRepository.getOrCreateKey(SECRET_KEY_ALIAS, true) assertThat(awaitItem()).isTrue() sessionObserver.onSessionDeleted("userId", wasLastSession = false) expectNoEvents() @@ -79,8 +82,10 @@ class DefaultLockScreenServiceTest { private fun TestScope.createDefaultLockScreenService( lockScreenConfig: LockScreenConfig = aLockScreenConfig(), lockScreenStore: LockScreenStore = InMemoryLockScreenStore(), + secretKeyRepository: SecretKeyRepository = SimpleSecretKeyRepository(), pinCodeManager: PinCodeManager = createDefaultPinCodeManager( lockScreenStore = lockScreenStore, + secretKeyRepository = secretKeyRepository, ), sessionObserver: SessionObserver = FakeSessionObserver(), appForegroundStateService: AppForegroundStateService = FakeAppForegroundStateService(), diff --git a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/biometric/FakeBiometricAuthenticator.kt b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/biometric/FakeBiometricAuthenticator.kt index 073bdc799d..63729f941a 100644 --- a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/biometric/FakeBiometricAuthenticator.kt +++ b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/biometric/FakeBiometricAuthenticator.kt @@ -12,6 +12,6 @@ class FakeBiometricAuthenticator( override val isActive: Boolean = false, private val authenticateLambda: suspend () -> BiometricAuthenticator.AuthenticationResult = { BiometricAuthenticator.AuthenticationResult.Success }, ) : BiometricAuthenticator { - override fun setup() = Unit + override suspend fun setup() = Unit override suspend fun authenticate() = authenticateLambda() } diff --git a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/pin/storage/InMemoryLockScreenStore.kt b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/pin/storage/InMemoryLockScreenStore.kt index 61acf71cdd..312a33b7f1 100644 --- a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/pin/storage/InMemoryLockScreenStore.kt +++ b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/pin/storage/InMemoryLockScreenStore.kt @@ -15,12 +15,7 @@ import kotlinx.coroutines.flow.MutableStateFlow private const val DEFAULT_REMAINING_ATTEMPTS = 3 class InMemoryLockScreenStore : LockScreenStore { - private val hasPinCode = MutableStateFlow(false) private var pinCode: String? = null - set(value) { - field = value - hasPinCode.value = value != null - } private var remainingAttempts: Int = DEFAULT_REMAINING_ATTEMPTS private var isBiometricUnlockAllowed = MutableStateFlow(false) @@ -48,10 +43,6 @@ class InMemoryLockScreenStore : LockScreenStore { pinCode = null } - override fun hasPinCode(): Flow { - return hasPinCode - } - override fun isBiometricUnlockAllowed(): Flow { return isBiometricUnlockAllowed } diff --git a/libraries/cryptography/api/build.gradle.kts b/libraries/cryptography/api/build.gradle.kts index 9ce26419d8..74fc5f6ecc 100644 --- a/libraries/cryptography/api/build.gradle.kts +++ b/libraries/cryptography/api/build.gradle.kts @@ -13,3 +13,7 @@ plugins { android { namespace = "io.element.android.libraries.cryptography.api" } + +dependencies { + implementation(libs.coroutines.core) +} diff --git a/libraries/cryptography/api/src/main/kotlin/io/element/android/libraries/cryptography/api/SecretKeyRepository.kt b/libraries/cryptography/api/src/main/kotlin/io/element/android/libraries/cryptography/api/SecretKeyRepository.kt index ba6c10dbe0..b210664d9f 100644 --- a/libraries/cryptography/api/src/main/kotlin/io/element/android/libraries/cryptography/api/SecretKeyRepository.kt +++ b/libraries/cryptography/api/src/main/kotlin/io/element/android/libraries/cryptography/api/SecretKeyRepository.kt @@ -8,6 +8,7 @@ package io.element.android.libraries.cryptography.api +import kotlinx.coroutines.flow.Flow import javax.crypto.SecretKey /** @@ -15,16 +16,18 @@ import javax.crypto.SecretKey * Implementation should be able to store the generated key securely. */ interface SecretKeyRepository { + fun hasKey(alias: String): Flow + /** * Get or create a secret key for a given alias. * @param alias the alias to use * @param requiresUserAuthentication true if the key should be protected by user authentication */ - fun getOrCreateKey(alias: String, requiresUserAuthentication: Boolean): SecretKey + suspend fun getOrCreateKey(alias: String, requiresUserAuthentication: Boolean): SecretKey /** * Delete the secret key for a given alias. * @param alias the alias to use */ - fun deleteKey(alias: String) + suspend fun deleteKey(alias: String) } diff --git a/libraries/cryptography/impl/build.gradle.kts b/libraries/cryptography/impl/build.gradle.kts index 454432de6f..3a1f55126e 100644 --- a/libraries/cryptography/impl/build.gradle.kts +++ b/libraries/cryptography/impl/build.gradle.kts @@ -21,6 +21,7 @@ setupDependencyInjection() dependencies { implementation(projects.libraries.di) + implementation(libs.coroutines.core) api(projects.libraries.cryptography.api) testCommonDependencies(libs) diff --git a/libraries/cryptography/impl/src/main/kotlin/io/element/android/libraries/cryptography/impl/KeyStoreSecretKeyRepository.kt b/libraries/cryptography/impl/src/main/kotlin/io/element/android/libraries/cryptography/impl/KeyStoreSecretKeyRepository.kt index 46572ef047..dccba2a136 100644 --- a/libraries/cryptography/impl/src/main/kotlin/io/element/android/libraries/cryptography/impl/KeyStoreSecretKeyRepository.kt +++ b/libraries/cryptography/impl/src/main/kotlin/io/element/android/libraries/cryptography/impl/KeyStoreSecretKeyRepository.kt @@ -13,11 +13,16 @@ import android.security.keystore.KeyGenParameterSpec import android.security.keystore.KeyProperties import dev.zacsweers.metro.AppScope import dev.zacsweers.metro.ContributesBinding +import dev.zacsweers.metro.SingleIn import io.element.android.libraries.cryptography.api.AESEncryptionSpecs import io.element.android.libraries.cryptography.api.SecretKeyRepository +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.asStateFlow import timber.log.Timber import java.security.KeyStore import java.security.KeyStoreException +import java.util.concurrent.ConcurrentHashMap import javax.crypto.KeyGenerator import javax.crypto.SecretKey @@ -25,13 +30,22 @@ import javax.crypto.SecretKey * Default implementation of [SecretKeyRepository] that uses the Android Keystore to store the keys. * The generated key uses AES algorithm, with a key size of 128 bits, and the GCM block mode. */ +@SingleIn(AppScope::class) @ContributesBinding(AppScope::class) class KeyStoreSecretKeyRepository( private val keyStore: KeyStore, ) : SecretKeyRepository { + private val hasKeyMap = ConcurrentHashMap>() + + override fun hasKey(alias: String): Flow { + return hasKeyMap.getOrPut(alias) { + MutableStateFlow(runCatching { keyStore.containsAlias(alias) }.getOrDefault(false)) + }.asStateFlow() + } + // False positive lint issue @SuppressLint("WrongConstant") - override fun getOrCreateKey(alias: String, requiresUserAuthentication: Boolean): SecretKey { + override suspend fun getOrCreateKey(alias: String, requiresUserAuthentication: Boolean): SecretKey { val secretKeyEntry = (keyStore.getEntry(alias, null) as? KeyStore.SecretKeyEntry) ?.secretKey return if (secretKeyEntry == null) { @@ -46,15 +60,22 @@ class KeyStoreSecretKeyRepository( .setUserAuthenticationRequired(requiresUserAuthentication) .build() generator.init(keyGenSpec) - generator.generateKey() + generator.generateKey().also { + hasKeyMap.getOrPut(alias) { + MutableStateFlow(true) + }.emit(true) + } } else { secretKeyEntry } } - override fun deleteKey(alias: String) { + override suspend fun deleteKey(alias: String) { try { keyStore.deleteEntry(alias) + hasKeyMap.getOrPut(alias) { + MutableStateFlow(false) + }.emit(false) } catch (e: KeyStoreException) { Timber.e(e) } diff --git a/libraries/cryptography/test/build.gradle.kts b/libraries/cryptography/test/build.gradle.kts index eaa621d53a..5cf04a9754 100644 --- a/libraries/cryptography/test/build.gradle.kts +++ b/libraries/cryptography/test/build.gradle.kts @@ -16,4 +16,5 @@ android { dependencies { api(projects.libraries.cryptography.api) + implementation(libs.coroutines.core) } diff --git a/libraries/cryptography/test/src/main/kotlin/io/element/android/libraries/cryptography/test/SimpleSecretKeyRepository.kt b/libraries/cryptography/test/src/main/kotlin/io/element/android/libraries/cryptography/test/SimpleSecretKeyRepository.kt index 0e301553ea..507325f45b 100644 --- a/libraries/cryptography/test/src/main/kotlin/io/element/android/libraries/cryptography/test/SimpleSecretKeyRepository.kt +++ b/libraries/cryptography/test/src/main/kotlin/io/element/android/libraries/cryptography/test/SimpleSecretKeyRepository.kt @@ -10,20 +10,39 @@ package io.element.android.libraries.cryptography.test import io.element.android.libraries.cryptography.api.AESEncryptionSpecs import io.element.android.libraries.cryptography.api.SecretKeyRepository +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.asStateFlow +import java.util.concurrent.ConcurrentHashMap import javax.crypto.KeyGenerator import javax.crypto.SecretKey class SimpleSecretKeyRepository : SecretKeyRepository { private var secretKeyForAlias = HashMap() - override fun getOrCreateKey(alias: String, requiresUserAuthentication: Boolean): SecretKey { + private val hasKeyMap = ConcurrentHashMap>() + + override fun hasKey(alias: String): Flow { + return hasKeyMap.getOrPut(alias) { + MutableStateFlow(false) + }.asStateFlow() + } + + override suspend fun getOrCreateKey(alias: String, requiresUserAuthentication: Boolean): SecretKey { return secretKeyForAlias.getOrPut(alias) { - generateKey() + generateKey().also { + hasKeyMap.getOrPut(alias) { + MutableStateFlow(true) + }.emit(true) + } } } - override fun deleteKey(alias: String) { + override suspend fun deleteKey(alias: String) { secretKeyForAlias.remove(alias) + hasKeyMap.getOrPut(alias) { + MutableStateFlow(false) + }.emit(false) } private fun generateKey(): SecretKey {