Address comments in #1924 (#1926)

Task/Issue URL: N/A

Description

Addressing all comments mentioned in #1924

Steps to test this PR

N/A

UI changes

N/A

Author: karlenDimla

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

@@ -18,31 +18,101 @@ package com.duckduckgo.securestorage.api
 
 import kotlinx.coroutines.flow.Flow
 
+/** Public API for the secure storage feature */
 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
+     */
     fun canAccessSecureStorage(): Boolean
 
+    /**
+     * This method authenticates the user without having to pass a user password. This method is relevant
+     * while the user password generation is still done programmatically within the secure storage. Once we support
+     * password creation, this method will be deprecated.
+     *
+     * @return Result.Success if no issues with authentication is encountered otherwise Result.Error
+     */
     suspend fun authenticateUser(): Result
 
+    /**
+     * This method authenticates the user using a provided [password].
+     *
+     * @return Result.Success if no issues with authentication is encountered otherwise Result.Error
+     */
     suspend fun authenticateUser(password: String): Result
 
+    /**
+     * This method adds a raw plaintext [WebsiteLoginCredentials] into the [SecureStorage]. This requires the user to be authenticated
+     * first see [authenticateUser].
+     *
+     * @throws UserNotAuthenticatedException if the user is not authenticated when calling this method
+     */
     @Throws(UserNotAuthenticatedException::class)
     suspend fun addWebsiteLoginCredential(websiteLoginCredentials: WebsiteLoginCredentials)
 
-    suspend fun getWebsiteLoginDetailsForDomain(domain: String): Flow<List<WebsiteLoginDetails>>
+    /**
+     * This method returns all [WebsiteLoginDetails] with the [domain] stored in the [SecureStorage].
+     * This does not require the user to be authenticated since [WebsiteLoginDetails] doesn't contain any L2 data.
+     *
+     * @return Flow<List<WebsiteLoginDetails>> a flow emitting a List of plain text WebsiteLoginDetails stored in SecureStorage.
+     */
+    suspend fun websiteLoginDetailsForDomain(domain: String): Flow<List<WebsiteLoginDetails>>
 
-    suspend fun getAllWebsiteLoginDetails(): Flow<List<WebsiteLoginDetails>>
+    /**
+     * This method returns all [WebsiteLoginDetails] stored in the [SecureStorage].
+     * This does not require the user to be authenticated since [WebsiteLoginDetails] doesn't contain any L2 data.
+     *
+     * @return Flow<List<WebsiteLoginDetails>> a flow containing a List of plain text WebsiteLoginDetails stored in SecureStorage.
+     */
+    suspend fun websiteLoginDetails(): Flow<List<WebsiteLoginDetails>>
 
+    /**
+     * This method returns the [WebsiteLoginCredentials] with the [id] stored in the [SecureStorage].
+     * This requires the user to be authenticated.
+     *
+     * @return [WebsiteLoginCredentials] containing the plaintext password
+     * @throws [UserNotAuthenticatedException] if the user is not authenticated when calling this method
+     */
     @Throws(UserNotAuthenticatedException::class)
     suspend fun getWebsiteLoginCredentials(id: Int): WebsiteLoginCredentials
 
+    /**
+     * This method returns the [WebsiteLoginCredentials] with the [domain] stored in the [SecureStorage].
+     * This requires the user to be authenticated.
+     *
+     * @return Flow<List<WebsiteLoginCredentials>>  a flow emitting a List of plain text WebsiteLoginCredentials stored in SecureStorage
+     * containing the plaintext password
+     * @throws [UserNotAuthenticatedException] if the user is not authenticated when calling this method
+     */
     @Throws(UserNotAuthenticatedException::class)
-    suspend fun getWebsiteLoginCredentialsForDomain(domain: String): Flow<List<WebsiteLoginCredentials>>
+    suspend fun websiteLoginCredentialsForDomain(domain: String): Flow<List<WebsiteLoginCredentials>>
 
+    /**
+     * This method returns all the [WebsiteLoginCredentials] stored in the [SecureStorage].
+     * This requires the user to be authenticated.
+     *
+     * @return Flow<List<WebsiteLoginCredentials>>  a flow emitting a List of plain text WebsiteLoginCredentials stored in SecureStorage
+     * containing the plaintext password
+     * @throws [UserNotAuthenticatedException] if the user is not authenticated when calling this method
+     */
     @Throws(UserNotAuthenticatedException::class)
-    suspend fun getAllWebsiteLoginCredentials(): Flow<List<WebsiteLoginCredentials>>
+    suspend fun websiteLoginCredentials(): Flow<List<WebsiteLoginCredentials>>
 
+    /**
+     * This method updates an existing [WebsiteLoginCredentials] in the [SecureStorage].
+     * This requires the user to be authenticated.
+     *
+     * @throws [UserNotAuthenticatedException] if the user is not authenticated when calling this method
+     */
     @Throws(UserNotAuthenticatedException::class)
     suspend fun updateWebsiteLoginCredentials(websiteLoginCredentials: WebsiteLoginCredentials)
 
+    /**
+     * This method removes an existing [WebsiteLoginCredentials] associated with an [id] from the [SecureStorage].
+     * This does not require the user to be authenticated.
+     */
     suspend fun deleteWebsiteLoginCredentials(id: Int)
 }

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

@@ -16,16 +16,42 @@
 
 package com.duckduckgo.securestorage.api
 
+/**
+ * Public sealed class representing the result of secure storage user authentication.
+ */
 sealed class Result {
+    /**
+     * Data class representing a success in secure storage authentication.
+     * [expiryInMillis] is the system time in millis to when the authentication is set to expire.
+     */
     data class Success(val expiryInMillis: Long) : Result()
+
+    /**
+     * Data class representing any error in secure storage authentication.
+     * [reason] provides more details on why the error occurred.
+     */
     data class Error(val reason: String) : Result()
 }
 
+/**
+ * Public data class that wraps all data related to a website login.
+ *
+ * [details] contains all l1 encrypted attributes
+ * [password] plain text password. All succeeding l2 and above attributes should be added here directly.
+ */
 data class WebsiteLoginCredentials(
     val details: WebsiteLoginDetails,
     val password: String?
 )
 
+/**
+ * Public data class that wraps all data that should only be covered with l1 encryption.
+ * All attributes is in plain text. Also, all should not require user authentication to be decrypted.
+ *
+ * [domain] url/name associated to a website login.
+ * [username] used to populate the username fields in a login
+ * [id] database id associated to the website login
+ */
 data class WebsiteLoginDetails(
     val domain: String?,
     val username: String?,

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

@@ -18,4 +18,8 @@ package com.duckduckgo.securestorage.api
 
 import java.lang.RuntimeException
 
+/**
+ * Public data class exception that is thrown when a method that requires user authentication is accessed by a
+ * non authenticated user.
+ */
 data class UserNotAuthenticatedException(override val message: String?) : RuntimeException()

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

@@ -26,13 +26,13 @@ dependencies {
     implementation project(path: ':appbuildconfig-api')
     implementation project(path: ':di')
     implementation project(path: ':secure-storage-api')
-    api project(path: ':secure-storage-store')
+    implementation project(path: ':secure-storage-store')
 
     implementation AndroidX.room.ktx
     implementation Google.dagger
     implementation KotlinX.coroutines.core
 
-    api "net.zetetic:android-database-sqlcipher:_"
+    implementation "net.zetetic:android-database-sqlcipher:_"
 }
 
 android {

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

@@ -32,9 +32,6 @@ import javax.inject.Inject
 class RealSecureStorage @Inject constructor(
     private val secureStorageRepository: SecureStorageRepository
 ) : SecureStorage {
-    companion object {
-        private const val DEFAULT_EXPIRY_IN_MILLIS = 30 * 60 * 1000
-    }
 
     override fun canAccessSecureStorage(): Boolean = true
 
@@ -53,15 +50,15 @@ class RealSecureStorage @Inject constructor(
         secureStorageRepository.addWebsiteLoginCredential(websiteLoginCredentials.toDataEntity())
     }
 
-    override suspend fun getWebsiteLoginDetailsForDomain(domain: String): Flow<List<WebsiteLoginDetails>> =
-        secureStorageRepository.getWebsiteLoginCredentialsForDomain(domain).map { list ->
+    override suspend fun websiteLoginDetailsForDomain(domain: String): Flow<List<WebsiteLoginDetails>> =
+        secureStorageRepository.websiteLoginCredentialsForDomain(domain).map { list ->
             list.map {
                 it.toDetails()
             }
         }
 
-    override suspend fun getAllWebsiteLoginDetails(): Flow<List<WebsiteLoginDetails>> =
-        secureStorageRepository.getAllWebsiteLoginCredentials().map { list ->
+    override suspend fun websiteLoginDetails(): Flow<List<WebsiteLoginDetails>> =
+        secureStorageRepository.websiteLoginCredentials().map { list ->
             list.map {
                 it.toDetails()
             }
@@ -70,17 +67,17 @@ class RealSecureStorage @Inject constructor(
     override suspend fun getWebsiteLoginCredentials(id: Int): WebsiteLoginCredentials =
         secureStorageRepository.getWebsiteLoginCredentialsForId(id).toCredentials()
 
-    override suspend fun getWebsiteLoginCredentialsForDomain(domain: String): Flow<List<WebsiteLoginCredentials>> =
+    override suspend fun websiteLoginCredentialsForDomain(domain: String): Flow<List<WebsiteLoginCredentials>> =
         // TODO (karl) Integrate L2 encryption
-        secureStorageRepository.getWebsiteLoginCredentialsForDomain(domain).map { list ->
+        secureStorageRepository.websiteLoginCredentialsForDomain(domain).map { list ->
             list.map {
                 it.toCredentials()
             }
         }
 
-    override suspend fun getAllWebsiteLoginCredentials(): Flow<List<WebsiteLoginCredentials>> =
+    override suspend fun websiteLoginCredentials(): Flow<List<WebsiteLoginCredentials>> =
         // TODO (karl) Integrate L2 encryption
-        secureStorageRepository.getAllWebsiteLoginCredentials().map { list ->
+        secureStorageRepository.websiteLoginCredentials().map { list ->
             list.map {
                 it.toCredentials()
             }
@@ -113,4 +110,8 @@ class RealSecureStorage @Inject constructor(
             username = username,
             id = id
         )
+
+    companion object {
+        private const val DEFAULT_EXPIRY_IN_MILLIS = 30 * 60 * 1000
+    }
 }

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

@@ -41,22 +41,17 @@ object SecureStorageModule {
     fun providesSecureStorageKeyStore(context: Context): SecureStorageKeyStore =
         RealSecureStorageKeyStore(context)
 
-    @Provides
-    fun providesSupportFactory(keyManager: SecureStorageKeyManager): SupportFactory {
-        return SupportFactory(keyManager.l1Key)
-    }
-
     @Provides
     @SingleInstanceIn(AppScope::class)
     fun providesSecureStorageDatabase(
         context: Context,
-        factory: SupportFactory
+        keyManager: SecureStorageKeyManager
     ): SecureStorageDatabase {
         return Room.databaseBuilder(
             context,
             SecureStorageDatabase::class.java,
             "secure_storage_database_encrypted.db"
-        ).openHelperFactory(factory)
+        ).openHelperFactory(SupportFactory(keyManager.l1Key))
             .addMigrations(*ALL_MIGRATIONS)
             .enableMultiInstanceInvalidation()
             .fallbackToDestructiveMigration()

secure-storage/secure-storage-store/build.gradle

@@ -36,6 +36,7 @@ dependencies {
     implementation AndroidX.core.ktx
     implementation AndroidX.room.ktx
     implementation AndroidX.security.crypto
+    implementation Square.okio
 
     kapt AndroidX.room.compiler
 }

secure-storage/secure-storage-store/src/main/java/com/duckduckgo/securestorage/store/SecureStorageKeyStore.kt

@@ -18,10 +18,11 @@ package com.duckduckgo.securestorage.store
 
 import android.content.Context
 import android.content.SharedPreferences
-import android.util.Base64
 import androidx.core.content.edit
 import androidx.security.crypto.EncryptedSharedPreferences
 import androidx.security.crypto.MasterKey
+import okio.ByteString.Companion.decodeBase64
+import okio.ByteString.Companion.toByteString
 import java.lang.Exception
 
 /**
@@ -54,12 +55,6 @@ interface SecureStorageKeyStore {
 class RealSecureStorageKeyStore constructor(
     private val context: Context
 ) : SecureStorageKeyStore {
-    companion object {
-        const val FILENAME = "com.duckduckgo.securestorage.store"
-        const val KEY_GENERATED_PASSWORD = "KEY_GENERATED_PASSWORD"
-        const val KEY_L1KEY = "KEY_L1KEY"
-        const val KEY_ENCRYPTED_L2KEY = "KEY_ENCRYPTED_L2KEY"
-    }
 
     private val encryptedPreferences: SharedPreferences? by lazy { encryptedPreferences() }
 
@@ -102,19 +97,24 @@ class RealSecureStorageKeyStore constructor(
         key: String,
         value: ByteArray?
     ) {
-        Base64.encodeToString(value, Base64.DEFAULT).also {
-            encryptedPreferences?.edit(commit = true) {
-                if (value == null) remove(key)
-                else putString(key, it)
-            }
+        encryptedPreferences?.edit(commit = true) {
+            if (value == null) remove(key)
+            else putString(key, value.toByteString().base64())
         }
     }
 
     private fun getValue(key: String): ByteArray? {
         return encryptedPreferences?.getString(key, null)?.run {
-            Base64.decode(this, Base64.DEFAULT)
+            this.decodeBase64()?.toByteArray()
         }
     }
 
     override fun canUseEncryption(): Boolean = encryptedPreferences != null
+
+    companion object {
+        const val FILENAME = "com.duckduckgo.securestorage.store"
+        const val KEY_GENERATED_PASSWORD = "KEY_GENERATED_PASSWORD"
+        const val KEY_L1KEY = "KEY_L1KEY"
+        const val KEY_ENCRYPTED_L2KEY = "KEY_ENCRYPTED_L2KEY"
+    }
 }

secure-storage/secure-storage-store/src/main/java/com/duckduckgo/securestorage/store/SecureStorageRepository.kt

@@ -28,11 +28,11 @@ interface SecureStorageRepository {
 
     suspend fun getWebsiteLoginCredentials(id: Int): WebsiteLoginCredentialsEntity
 
-    suspend fun getWebsiteLoginCredentialsForDomain(domain: String): Flow<List<WebsiteLoginCredentialsEntity>>
+    suspend fun websiteLoginCredentialsForDomain(domain: String): Flow<List<WebsiteLoginCredentialsEntity>>
 
     suspend fun getWebsiteLoginCredentialsForId(id: Int): WebsiteLoginCredentialsEntity
 
-    suspend fun getAllWebsiteLoginCredentials(): Flow<List<WebsiteLoginCredentialsEntity>>
+    suspend fun websiteLoginCredentials(): Flow<List<WebsiteLoginCredentialsEntity>>
 
     suspend fun updateWebsiteLoginCredentials(websiteLoginCredentials: WebsiteLoginCredentialsEntity)
 
@@ -49,11 +49,11 @@ class RealSecureStorageRepository constructor(
     override suspend fun getWebsiteLoginCredentials(id: Int): WebsiteLoginCredentialsEntity =
         websiteLoginCredentialsDao.getWebsiteLoginCredentialsById(id)
 
-    override suspend fun getWebsiteLoginCredentialsForDomain(domain: String): Flow<List<WebsiteLoginCredentialsEntity>> =
-        websiteLoginCredentialsDao.getWebsiteLoginCredentialsByDomain(domain)
+    override suspend fun websiteLoginCredentialsForDomain(domain: String): Flow<List<WebsiteLoginCredentialsEntity>> =
+        websiteLoginCredentialsDao.websiteLoginCredentialsByDomain(domain)
 
-    override suspend fun getAllWebsiteLoginCredentials(): Flow<List<WebsiteLoginCredentialsEntity>> =
-        websiteLoginCredentialsDao.getWebsiteLoginCredentials()
+    override suspend fun websiteLoginCredentials(): Flow<List<WebsiteLoginCredentialsEntity>> =
+        websiteLoginCredentialsDao.websiteLoginCredentials()
 
     override suspend fun getWebsiteLoginCredentialsForId(id: Int): WebsiteLoginCredentialsEntity =
         websiteLoginCredentialsDao.getWebsiteLoginCredentialsById(id)

secure-storage/secure-storage-store/src/main/java/com/duckduckgo/securestorage/store/db/WebsiteLoginCredentialsDao.kt

@@ -34,10 +34,10 @@ interface WebsiteLoginCredentialsDao {
     fun update(loginCredentials: WebsiteLoginCredentialsEntity)
 
     @Query("select * from website_login_credentials")
-    fun getWebsiteLoginCredentials(): Flow<List<WebsiteLoginCredentialsEntity>>
+    fun websiteLoginCredentials(): Flow<List<WebsiteLoginCredentialsEntity>>
 
     @Query("select * from website_login_credentials where domain = :domain")
-    fun getWebsiteLoginCredentialsByDomain(domain: String): Flow<List<WebsiteLoginCredentialsEntity>>
+    fun websiteLoginCredentialsByDomain(domain: String): Flow<List<WebsiteLoginCredentialsEntity>>
 
     @Query("select * from website_login_credentials where id = :id")
     fun getWebsiteLoginCredentialsById(id: Int): WebsiteLoginCredentialsEntity