Add tests to secure-storage (#2000)

Task/Issue URL: https://app.asana.com/0/0/1202253978523860/f
Stack on: #1980

Description

This PR includes a few refactoring and addition for tests for secure storage classes.

Steps to test this PR

 Sanity test autofill

modules/autofill/autofill-store/src/main/java/com/duckduckgo/autofill/store/SecureStoreBackedAutofillStore.kt

@@ -20,7 +20,7 @@ import com.duckduckgo.app.global.extractSchemeAndDomain
 import com.duckduckgo.autofill.domain.app.LoginCredentials
 import com.duckduckgo.di.scopes.AppScope
 import com.duckduckgo.securestorage.api.SecureStorage
-import com.duckduckgo.securestorage.api.WebsiteLoginCredentials
+import com.duckduckgo.securestorage.api.WebsiteLoginDetailsWithCredentials
 import com.duckduckgo.securestorage.api.WebsiteLoginDetails
 import com.squareup.anvil.annotations.ContributesTo
 import dagger.Module
@@ -35,7 +35,7 @@ class SecureStoreBackedAutofillStore(val secureStorage: SecureStorage) : Autofil
         Timber.i("Querying secure store for stored credentials. rawUrl: %s, extractedDomain:%s", rawUrl, rawUrl.extractSchemeAndDomain())
         val url = rawUrl.extractSchemeAndDomain() ?: return emptyList()
 
-        val storedCredentials = secureStorage.websiteLoginCredentialsForDomain(url).firstOrNull() ?: emptyList()
+        val storedCredentials = secureStorage.websiteLoginDetailsWithCredentialsForDomain(url).firstOrNull() ?: emptyList()
         Timber.v("Found %d credentials for %s", storedCredentials.size, url)
 
         return storedCredentials.map { it.toLoginCredentials() }
@@ -54,27 +54,27 @@ class SecureStoreBackedAutofillStore(val secureStorage: SecureStorage) : Autofil
         Timber.i("Saving login credentials for %s. username=%s", url, credentials.username)
 
         val loginDetails = WebsiteLoginDetails(domain = url, username = credentials.username)
-        val webSiteLoginCredentials = WebsiteLoginCredentials(loginDetails, password = credentials.password)
+        val webSiteLoginCredentials = WebsiteLoginDetailsWithCredentials(loginDetails, password = credentials.password)
 
-        secureStorage.addWebsiteLoginCredential(webSiteLoginCredentials)
+        secureStorage.addWebsiteLoginDetailsWithCredentials(webSiteLoginCredentials)
     }
 
     override suspend fun getAllCredentials(): Flow<List<LoginCredentials>> {
-        return secureStorage.websiteLoginCredentials()
+        return secureStorage.websiteLoginDetailsWithCredentials()
             .map { list ->
                 list.map { it.toLoginCredentials() }
             }
     }
 
     override suspend fun deleteCredentials(id: Int) {
-        secureStorage.deleteWebsiteLoginCredentials(id)
+        secureStorage.deleteWebsiteLoginDetailsWithCredentials(id)
     }
 
     override suspend fun updateCredentials(credentials: LoginCredentials) {
-        secureStorage.updateWebsiteLoginCredentials(credentials.toWebsiteLoginCredentials())
+        secureStorage.updateWebsiteLoginDetailsWithCredentials(credentials.toWebsiteLoginCredentials())
     }
 
-    private fun WebsiteLoginCredentials.toLoginCredentials(): LoginCredentials {
+    private fun WebsiteLoginDetailsWithCredentials.toLoginCredentials(): LoginCredentials {
         return LoginCredentials(
             id = details.id,
             domain = details.domain,
@@ -83,8 +83,8 @@ class SecureStoreBackedAutofillStore(val secureStorage: SecureStorage) : Autofil
         )
     }
 
-    private fun LoginCredentials.toWebsiteLoginCredentials(): WebsiteLoginCredentials {
-        return WebsiteLoginCredentials(
+    private fun LoginCredentials.toWebsiteLoginCredentials(): WebsiteLoginDetailsWithCredentials {
+        return WebsiteLoginDetailsWithCredentials(
             details = WebsiteLoginDetails(domain = domain, username = username, id = id),
             password = password
         )

secure-storage/secure-storage-api/src/main/java/com/duckduckgo/securestorage/api/SecureStorage.kt

@@ -24,17 +24,17 @@ interface SecureStorage {
     /**
      * This method can be used to check if the secure storage has been instantiated properly.
      *
-     * @return `true` if all dependencies of SecureStorage have been instantiated properly otherwise `false
+     * @return `true` if all dependencies of SecureStorage have been instantiated properly otherwise `false`
      */
     fun canAccessSecureStorage(): Boolean
 
     /**
-     * This method adds a raw plaintext [WebsiteLoginCredentials] into the [SecureStorage].
+     * This method adds a raw plaintext [WebsiteLoginDetailsWithCredentials] into the [SecureStorage].
      *
      * @throws [SecureStorageException] if something went wrong while trying to perform the action. See type to get more info on the cause.
      */
     @Throws(SecureStorageException::class)
-    suspend fun addWebsiteLoginCredential(websiteLoginCredentials: WebsiteLoginCredentials)
+    suspend fun addWebsiteLoginDetailsWithCredentials(websiteLoginDetailsWithCredentials: WebsiteLoginDetailsWithCredentials)
 
     /**
      * This method returns all [WebsiteLoginDetails] with the [domain] stored in the [SecureStorage].
@@ -53,47 +53,47 @@ interface SecureStorage {
     suspend fun websiteLoginDetails(): Flow<List<WebsiteLoginDetails>>
 
     /**
-     * This method returns the [WebsiteLoginCredentials] with the [id] stored in the [SecureStorage].
+     * This method returns the [WebsiteLoginDetailsWithCredentials] with the [id] stored in the [SecureStorage].
      * This returns decrypted sensitive data (encrypted in L2). Use this only when sensitive data is needed to be accessed.
      *
-     * @return [WebsiteLoginCredentials] containing the plaintext password
+     * @return [WebsiteLoginDetailsWithCredentials] containing the plaintext password
      * @throws [SecureStorageException] if something went wrong while trying to perform the action. See type to get more info on the cause.
      */
     @Throws(SecureStorageException::class)
-    suspend fun getWebsiteLoginCredentials(id: Int): WebsiteLoginCredentials
+    suspend fun getWebsiteLoginDetailsWithCredentials(id: Int): WebsiteLoginDetailsWithCredentials
 
     /**
-     * This method returns the [WebsiteLoginCredentials] with the [domain] stored in the [SecureStorage].
+     * This method returns the [WebsiteLoginDetailsWithCredentials] with the [domain] stored in the [SecureStorage].
      * This returns decrypted sensitive data (encrypted in L2). Use this only when sensitive data is needed to be accessed.
      *
-     * @return Flow<List<WebsiteLoginCredentials>>  a flow emitting a List of plain text WebsiteLoginCredentials stored in SecureStorage
-     * containing the plaintext password
+     * @return Flow<List<WebsiteLoginDetailsWithCredentials>> a flow emitting a List of plain text WebsiteLoginDetailsWithCredentials stored
+     * in SecureStorage containing the plaintext password
      * @throws [SecureStorageException] if something went wrong while trying to perform the action. See type to get more info on the cause.
      */
     @Throws(SecureStorageException::class)
-    suspend fun websiteLoginCredentialsForDomain(domain: String): Flow<List<WebsiteLoginCredentials>>
+    suspend fun websiteLoginDetailsWithCredentialsForDomain(domain: String): Flow<List<WebsiteLoginDetailsWithCredentials>>
 
     /**
-     * This method returns all the [WebsiteLoginCredentials] stored in the [SecureStorage].
+     * This method returns all the [WebsiteLoginDetailsWithCredentials] stored in the [SecureStorage].
      * This returns decrypted sensitive data (encrypted in L2). Use this only when sensitive data is needed to be accessed.
      *
-     * @return Flow<List<WebsiteLoginCredentials>>  a flow emitting a List of plain text WebsiteLoginCredentials stored in SecureStorage
-     * containing the plaintext password
+     * @return Flow<List<WebsiteLoginDetailsWithCredentials>>  a flow emitting a List of plain text WebsiteLoginDetailsWithCredentials stored
+     * in SecureStorage containing the plaintext password
      * @throws [SecureStorageException] if something went wrong while trying to perform the action. See type to get more info on the cause.
      */
     @Throws(SecureStorageException::class)
-    suspend fun websiteLoginCredentials(): Flow<List<WebsiteLoginCredentials>>
+    suspend fun websiteLoginDetailsWithCredentials(): Flow<List<WebsiteLoginDetailsWithCredentials>>
 
     /**
-     * This method updates an existing [WebsiteLoginCredentials] in the [SecureStorage].
+     * This method updates an existing [WebsiteLoginDetailsWithCredentials] in the [SecureStorage].
      *
      * @throws [SecureStorageException] if something went wrong while trying to perform the action. See type to get more info on the cause.
      */
     @Throws(SecureStorageException::class)
-    suspend fun updateWebsiteLoginCredentials(websiteLoginCredentials: WebsiteLoginCredentials)
+    suspend fun updateWebsiteLoginDetailsWithCredentials(websiteLoginDetailsWithCredentials: WebsiteLoginDetailsWithCredentials)
 
     /**
-     * This method removes an existing [WebsiteLoginCredentials] associated with an [id] from the [SecureStorage].
+     * This method removes an existing [WebsiteLoginDetailsWithCredentials] associated with an [id] from the [SecureStorage].
      */
-    suspend fun deleteWebsiteLoginCredentials(id: Int)
+    suspend fun deleteWebsiteLoginDetailsWithCredentials(id: Int)
 }

secure-storage/secure-storage-api/src/main/java/com/duckduckgo/securestorage/api/SecureStorageModels.kt

@@ -22,7 +22,7 @@ package com.duckduckgo.securestorage.api
  * [details] contains all l1 encrypted attributes
  * [password] plain text password. All succeeding l2 and above attributes should be added here directly.
  */
-data class WebsiteLoginCredentials(
+data class WebsiteLoginDetailsWithCredentials(
     val details: WebsiteLoginDetails,
     val password: String?
 )

secure-storage/secure-storage-impl/build.gradle

@@ -35,6 +35,15 @@ dependencies {
     implementation Square.okio
 
     implementation "net.zetetic:android-database-sqlcipher:_"
+
+    testImplementation project(path: ':common-test')
+    testImplementation Testing.junit4
+    testImplementation "org.mockito.kotlin:mockito-kotlin:_"
+    testImplementation (KotlinX.coroutines.test) {
+        // https://github.com/Kotlin/kotlinx.coroutines/issues/2023
+        // conflicts with mockito due to direct inclusion of byte buddy
+        exclude group: "org.jetbrains.kotlinx", module: "kotlinx-coroutines-debug"
+    }
 }
 
 android {

secure-storage/secure-storage-impl/src/main/java/com/duckduckgo/securestorage/impl/DerivedKeySecretFactory.kt

@@ -0,0 +1,41 @@
+/*
+ * Copyright (c) 2022 DuckDuckGo
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.duckduckgo.securestorage.impl
+
+import java.security.Key
+import javax.crypto.SecretKeyFactory
+import javax.crypto.spec.PBEKeySpec
+
+interface DerivedKeySecretFactory {
+    fun getKey(spec: PBEKeySpec): Key
+}
+
+class RealDerivedKeySecretFactory : DerivedKeySecretFactory {
+    private val secretKeyFactory by lazy {
+        SecretKeyFactory.getInstance("PBKDF2withHmacSHA256")
+    }
+
+    override fun getKey(spec: PBEKeySpec): Key = secretKeyFactory.generateSecret(spec)
+}
+
+class LegacyDerivedKeySecretFactory : DerivedKeySecretFactory {
+    private val secretKeyFactory by lazy {
+        SecretKeyFactory.getInstance("PBKDF2withHmacSHA1")
+    }
+
+    override fun getKey(spec: PBEKeySpec): Key = secretKeyFactory.generateSecret(spec)
+}

secure-storage/secure-storage-impl/src/main/java/com/duckduckgo/securestorage/impl/RealSecureStorage.kt

@@ -19,7 +19,7 @@ package com.duckduckgo.securestorage.impl
 import com.duckduckgo.app.global.DispatcherProvider
 import com.duckduckgo.di.scopes.AppScope
 import com.duckduckgo.securestorage.api.SecureStorage
-import com.duckduckgo.securestorage.api.WebsiteLoginCredentials
+import com.duckduckgo.securestorage.api.WebsiteLoginDetailsWithCredentials
 import com.duckduckgo.securestorage.api.WebsiteLoginDetails
 import com.duckduckgo.securestorage.impl.encryption.EncryptionHelper.EncryptedString
 import com.duckduckgo.securestorage.store.SecureStorageRepository
@@ -39,9 +39,9 @@ class RealSecureStorage @Inject constructor(
 
     override fun canAccessSecureStorage(): Boolean = l2DataTransformer.canProcessData()
 
-    override suspend fun addWebsiteLoginCredential(websiteLoginCredentials: WebsiteLoginCredentials) {
+    override suspend fun addWebsiteLoginDetailsWithCredentials(websiteLoginDetailsWithCredentials: WebsiteLoginDetailsWithCredentials) {
         withContext(dispatchers.io()) {
-            secureStorageRepository.addWebsiteLoginCredential(websiteLoginCredentials.toDataEntity())
+            secureStorageRepository.addWebsiteLoginCredential(websiteLoginDetailsWithCredentials.toDataEntity())
         }
     }
 
@@ -63,12 +63,12 @@ class RealSecureStorage @Inject constructor(
             }
         }
 
-    override suspend fun getWebsiteLoginCredentials(id: Int): WebsiteLoginCredentials =
+    override suspend fun getWebsiteLoginDetailsWithCredentials(id: Int): WebsiteLoginDetailsWithCredentials =
         withContext(dispatchers.io()) {
             secureStorageRepository.getWebsiteLoginCredentialsForId(id).toCredentials()
         }
 
-    override suspend fun websiteLoginCredentialsForDomain(domain: String): Flow<List<WebsiteLoginCredentials>> =
+    override suspend fun websiteLoginDetailsWithCredentialsForDomain(domain: String): Flow<List<WebsiteLoginDetailsWithCredentials>> =
         withContext(dispatchers.io()) {
             secureStorageRepository.websiteLoginCredentialsForDomain(domain).map { list ->
                 list.map {
@@ -77,7 +77,7 @@ class RealSecureStorage @Inject constructor(
             }
         }
 
-    override suspend fun websiteLoginCredentials(): Flow<List<WebsiteLoginCredentials>> =
+    override suspend fun websiteLoginDetailsWithCredentials(): Flow<List<WebsiteLoginDetailsWithCredentials>> =
         withContext(dispatchers.io()) {
             secureStorageRepository.websiteLoginCredentials().map { list ->
                 list.map {
@@ -86,17 +86,17 @@ class RealSecureStorage @Inject constructor(
             }
         }
 
-    override suspend fun updateWebsiteLoginCredentials(websiteLoginCredentials: WebsiteLoginCredentials) =
+    override suspend fun updateWebsiteLoginDetailsWithCredentials(websiteLoginDetailsWithCredentials: WebsiteLoginDetailsWithCredentials) =
         withContext(dispatchers.io()) {
-            secureStorageRepository.updateWebsiteLoginCredentials(websiteLoginCredentials.toDataEntity())
+            secureStorageRepository.updateWebsiteLoginCredentials(websiteLoginDetailsWithCredentials.toDataEntity())
         }
 
-    override suspend fun deleteWebsiteLoginCredentials(id: Int) =
+    override suspend fun deleteWebsiteLoginDetailsWithCredentials(id: Int) =
         withContext(dispatchers.io()) {
             secureStorageRepository.deleteWebsiteLoginCredentials(id)
         }
 
-    private fun WebsiteLoginCredentials.toDataEntity(): WebsiteLoginCredentialsEntity {
+    private fun WebsiteLoginDetailsWithCredentials.toDataEntity(): WebsiteLoginCredentialsEntity {
         val encryptedData = encryptData(password)
         return WebsiteLoginCredentialsEntity(
             id = details.id ?: 0,
@@ -107,8 +107,8 @@ class RealSecureStorage @Inject constructor(
         )
     }
 
-    private fun WebsiteLoginCredentialsEntity.toCredentials(): WebsiteLoginCredentials =
-        WebsiteLoginCredentials(
+    private fun WebsiteLoginCredentialsEntity.toCredentials(): WebsiteLoginDetailsWithCredentials =
+        WebsiteLoginDetailsWithCredentials(
             details = toDetails(),
             password = decryptData(password, iv)
         )

secure-storage/secure-storage-impl/src/main/java/com/duckduckgo/securestorage/impl/SecureStorageKeyGenerator.kt

@@ -23,10 +23,11 @@ import com.duckduckgo.di.scopes.AppScope
 import com.squareup.anvil.annotations.ContributesBinding
 import java.security.Key
 import javax.crypto.KeyGenerator
-import javax.crypto.SecretKeyFactory
 import javax.crypto.spec.PBEKeySpec
 import javax.crypto.spec.SecretKeySpec
 import javax.inject.Inject
+import javax.inject.Named
+import javax.inject.Provider
 
 interface SecureStorageKeyGenerator {
     fun generateKey(): Key
@@ -39,22 +40,16 @@ interface SecureStorageKeyGenerator {
 
 @ContributesBinding(AppScope::class)
 class RealSecureStorageKeyGenerator @Inject constructor(
-    private val appBuildConfig: AppBuildConfig
+    private val appBuildConfig: AppBuildConfig,
+    @Named("DerivedKeySecretFactoryFor26Up") private val derivedKeySecretFactory: Provider<DerivedKeySecretFactory>,
+    @Named("DerivedKeySecretFactoryForLegacy") private val legacyDerivedKeySecretFactory: Provider<DerivedKeySecretFactory>,
 ) : SecureStorageKeyGenerator {
     private val keyGenerator: KeyGenerator by lazy {
         KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES).also {
             it.init(SIZE)
         }
     }
 
-    private val passwordSecretKeyFactory by lazy {
-        SecretKeyFactory.getInstance("PBKDF2withHmacSHA256")
-    }
-
-    private val legacyPasswordSecretKeyFactory by lazy {
-        SecretKeyFactory.getInstance("PBKDF2withHmacSHA1")
-    }
-
     override fun generateKey(): Key = keyGenerator.generateKey()
 
     override fun generateKeyFromKeyMaterial(keyMaterial: ByteArray): Key = SecretKeySpec(
@@ -67,7 +62,7 @@ class RealSecureStorageKeyGenerator @Inject constructor(
         salt: ByteArray
     ): Key =
         if (appBuildConfig.sdkInt >= Build.VERSION_CODES.O) {
-            passwordSecretKeyFactory.generateSecret(
+            derivedKeySecretFactory.get().getKey(
                 PBEKeySpec(
                     password.toCharArray(),
                     salt,
@@ -76,7 +71,7 @@ class RealSecureStorageKeyGenerator @Inject constructor(
                 )
             )
         } else {
-            legacyPasswordSecretKeyFactory.generateSecret(
+            legacyDerivedKeySecretFactory.get().getKey(
                 PBEKeySpec(
                     password.toCharArray(),
                     salt,