Fix code review changes

neelanshsahai · neelanshsahai · commit f3341697af84 · 2025-09-19T00:44:47.000+05:30
Change-Id: I0571c151e62851634783b2aa8943b47fb84eea36
diff --git a/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/AppDependencies.kt b/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/AppDependencies.kt
@@ -23,6 +23,9 @@ import com.example.android.authentication.myvault.data.CredentialsDataSource
 import com.example.android.authentication.myvault.data.CredentialsRepository
 import com.example.android.authentication.myvault.data.RPIconDataSource
 import com.example.android.authentication.myvault.data.room.MyVaultDatabase
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.SupervisorJob
 
 /**
  * This class is an application-level singleton object which is providing dependencies required for the app to function.
@@ -42,6 +45,8 @@ object AppDependencies {
 
     lateinit var rpIconDataSource: RPIconDataSource
 
+    lateinit var coroutineScope: CoroutineScope
+
     /**
      * Initializes the core components required for the application's data storage and icon handling.
      * This includes:
@@ -72,5 +77,7 @@ object AppDependencies {
                 credentialsDataSource,
                 context,
             )
+
+        coroutineScope = CoroutineScope(Dispatchers.Default + SupervisorJob())
     }
 }
diff --git a/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/NotificationUtils.kt b/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/NotificationUtils.kt
@@ -13,6 +13,18 @@ import androidx.core.app.NotificationCompat
 import androidx.core.app.NotificationManagerCompat
 import com.example.android.authentication.myvault.ui.MainActivity
 
+/**
+ * Creates and registers a notification channel with the system.
+ *
+ * This is a utility extension function that creates a Notification Channel with
+ * [NotificationManager.IMPORTANCE_HIGH] to show pop-up notification on receiving
+ * signals from the RP apps
+ *
+ * @param channelName The user-visible name of the channel.
+ *                    This is displayed in the system's notification settings.
+ * @param channelDescription The user-visible description of the channel.
+ *                           This is displayed in the system's notification settings.
+ */
 fun Context.createNotificationChannel(
     channelName: String,
     channelDescription: String,
@@ -24,11 +36,17 @@ fun Context.createNotificationChannel(
     ).apply {
         description = channelDescription
     }
-    // Register the channel with the system.
+
     val notificationManager = getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager
     notificationManager.createNotificationChannel(channel)
 }
 
+/**
+ * Utility extension function that displays a system notification with the given title and content.
+ *
+ * @param title The title of the notification.
+ * @param content The main content text of the notification.
+ */
 fun Context.showNotification(
     title: String,
     content: String,
diff --git a/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/data/CredentialProviderService.kt b/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/data/CredentialProviderService.kt
@@ -1,7 +1,6 @@
 package com.example.android.authentication.myvault.data
 
 import android.annotation.SuppressLint
-import android.util.Log
 import androidx.credentials.SignalAllAcceptedCredentialIdsRequest
 import androidx.credentials.SignalCurrentUserDetailsRequest
 import androidx.credentials.SignalUnknownCredentialRequest
@@ -16,91 +15,211 @@ import com.example.android.authentication.myvault.NAME
 import com.example.android.authentication.myvault.R
 import com.example.android.authentication.myvault.USER_ID
 import com.example.android.authentication.myvault.showNotification
-import kotlinx.coroutines.runBlocking
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.launch
+import kotlinx.coroutines.withContext
 import org.json.JSONArray
 import org.json.JSONObject
 
+/**
+ * A service that listens to credential provider events triggered by the relying parties
+ *
+ * This service is responsible for handling signals related to credential state changes in the RPs,
+ * such as when a credential is no longer valid, when a list of accepted credentials is accepted,
+ * or when current user details change for credentials
+ */
 class CredentialProviderService: CredentialProviderEventsService() {
     private val dataSource = AppDependencies.credentialsDataSource
+    private val coroutineScope = AppDependencies.coroutineScope
 
+    /**
+     * Called when the system or another credential provider signals a change in credential state.
+     *
+     * This method inspects the type of [ProviderSignalCredentialStateRequest] and delegates
+     * to the appropriate handler function to update the local data store and show a notification.
+     * After processing the signal, {@link ProviderSignalCredentialStateCallback#onSignalConsumed()}
+     * is called to acknowledge receipt of the signal.
+     *
+     * The {@link SuppressLint("RestrictedApi")} annotation is used because this method
+     * interacts with APIs from the {@code androidx.credentials} library that might be
+     * marked as restricted for extension by library developers.
+     *
+     * @param request The request containing details about the credential state signal.
+     * @param callback The callback to be invoked after the signal has been processed.
+     */
     @SuppressLint("RestrictedApi")
     override fun onSignalCredentialStateRequest(
         request: ProviderSignalCredentialStateRequest,
         callback: ProviderSignalCredentialStateCallback,
     ) {
         when (request.callingRequest) {
             is SignalUnknownCredentialRequest -> {
-                handleUnknownCredentialRequest(request.callingRequest.requestJson)
-                showNotification(
-                    getString(R.string.credential_deletion),
-                    getString(R.string.unknown_signal_message)
+                updateDataOnSignalAndShowNotification(
+                    handleRequest = ::handleUnknownCredentialRequest,
+                    requestJson = request.callingRequest.requestJson,
+                    notificationTitle = getString(R.string.credential_deletion),
+                    notificationContent = getString(R.string.unknown_signal_message)
                 )
             }
 
             is SignalAllAcceptedCredentialIdsRequest -> {
-                handleAcceptedCredentialsRequest(request.callingRequest.requestJson)
-                showNotification(
-                    getString(R.string.credentials_list_updation),
-                    getString(R.string.all_accepted_signal_message)
+                updateDataOnSignalAndShowNotification(
+                    handleRequest = ::handleAcceptedCredentialsRequest,
+                    requestJson = request.callingRequest.requestJson,
+                    notificationTitle = getString(R.string.credentials_list_updation),
+                    notificationContent = getString(R.string.all_accepted_signal_message)
                 )
             }
+
             is SignalCurrentUserDetailsRequest -> {
-                handleCurrentUserDetailRequest(request.callingRequest.requestJson)
-                showNotification(
-                    getString(R.string.user_details_updation),
-                    getString(R.string.current_user_signal_message)
+                updateDataOnSignalAndShowNotification(
+                    handleRequest = ::handleCurrentUserDetailRequest,
+                    requestJson = request.callingRequest.requestJson,
+                    notificationTitle = getString(R.string.user_details_updation),
+                    notificationContent = getString(R.string.current_user_signal_message)
                 )
             }
+
             else -> { }
         }
 
         callback.onSignalConsumed()
     }
 
-    private fun handleUnknownCredentialRequest(requestJson: String) = runBlocking {
-        val credentialId = JSONObject(requestJson).getString(CREDENTIAL_ID)
-        dataSource.getPasskey(credentialId)?.let {
-            dataSource.hidePasskey(it)
+    /**
+     * A helper function to asynchronously handle a credential state update request,
+     * update the data source, and then show a system notification on the main thread.
+     *
+     * @param handleRequest A suspend function that takes the request JSON string and processes it.
+     *                      This function is responsible for interacting with the data source.
+     * @param requestJson The JSON string payload from the original credential signal request.
+     * @param notificationTitle The title to be used for the system notification.
+     * @param notificationContent The content text for the system notification.
+     */
+    private fun updateDataOnSignalAndShowNotification(
+        handleRequest: suspend (String) -> Boolean,
+        requestJson: String,
+        notificationTitle: String,
+        notificationContent: String,
+    ) {
+        coroutineScope.launch {
+            val success = handleRequest(requestJson)
+            withContext(Dispatchers.Main) {
+                if (success) {
+                    showNotification(
+                        title = notificationTitle,
+                        content = notificationContent,
+                    )
+                }
+            }
+        }
+    }
+
+    /**
+     * Handles a [SignalUnknownCredentialRequest] by parsing the credential ID
+     * from the request JSON and attempting to hide the corresponding passkey in the data source.
+     *
+     * "Hiding" a passkey typically means marking it as inactive or not to be suggested
+     * for autofill, often because the system has indicated it's no longer valid
+     * (e.g., deleted from the authenticator).
+     *
+     * @param requestJson The JSON string payload from the [SignalUnknownCredentialRequest].
+     *                   Expected to contain a {@code CREDENTIAL_ID}.
+     */
+    private suspend fun handleUnknownCredentialRequest(requestJson: String): Boolean {
+        try {
+            val credentialId = JSONObject(requestJson).getString(CREDENTIAL_ID)
+            dataSource.getPasskey(credentialId)?.let {
+                // Currently hiding the passkey on UnknownSignal for testing purpose
+                // If the business logc requires deletion, please add deletion code instead
+                dataSource.hidePasskey(it)
+            }
+            return true
+        } catch (e: Exception) {
+            e.printStackTrace()
+            return false
         }
     }
 
-    private fun handleAcceptedCredentialsRequest(requestJson: String) = runBlocking {
-        val request = JSONObject(requestJson)
-        val userId = request.getString(USER_ID)
-        val listCurrentPasskeysForUser = dataSource.getPasskeyForUser(userId) ?: emptyList()
-        val listAllAcceptedCredIds = mutableListOf<String>()
-        when (val value = request.get(ACCEPTED_CREDENTIAL_IDS)) {
-            is String -> listAllAcceptedCredIds.add(value)
-            is JSONArray -> {
-                for (i in 0 until value.length()) {
-                    val item = value.get(i)
-                    if (item is String) {
-                        listAllAcceptedCredIds.add(item)
+    /**
+     * Handles a {@link SignalAllAcceptedCredentialIdsRequest} by synchronizing the visibility
+     * state of passkeys for a specific user.
+     *
+     * It retrieves all current passkeys for the user from the data source. Then, it compares
+     * this list against the list of accepted credential IDs provided in the signal.
+     * Passkeys whose IDs are in the accepted list are unhidden (made active).
+     * Passkeys whose IDs are not in the accepted list are hidden (made inactive).
+     *
+     * This is useful for scenarios where the system provides an authoritative list of
+     * credentials that are currently valid or preferred for a user.
+     *
+     * @param requestJson The JSON string payload from the {@link SignalAllAcceptedCredentialIdsRequest}.
+     *                   Expected to contain a {@code USER_ID} and {@code ACCEPTED_CREDENTIAL_IDS}
+     *                   (which can be a string or a JSON array of strings).
+     */
+    private suspend fun handleAcceptedCredentialsRequest(requestJson: String): Boolean {
+        try {
+            val request = JSONObject(requestJson)
+            val userId = request.getString(USER_ID)
+            val listCurrentPasskeysForUser = dataSource.getAllPasskeysForUser(userId) ?: emptyList()
+            val listAllAcceptedCredIds = mutableListOf<String>()
+            when (val value = request.get(ACCEPTED_CREDENTIAL_IDS)) {
+                is String -> listAllAcceptedCredIds.add(value)
+                is JSONArray -> {
+                    for (i in 0 until value.length()) {
+                        val item = value.get(i)
+                        if (item is String) {
+                            listAllAcceptedCredIds.add(item)
+                        }
                     }
                 }
+
+                else -> { /*do nothing*/
+                }
             }
-            else -> { /*do nothing*/ }
-        }
 
-        for (key in listCurrentPasskeysForUser) {
-            if (listAllAcceptedCredIds.contains(key.credId)) {
-                dataSource.unhidePasskey(key)
-            } else {
-                dataSource.hidePasskey(key)
+            for (key in listCurrentPasskeysForUser) {
+                if (listAllAcceptedCredIds.contains(key.credId)) {
+                    dataSource.unhidePasskey(key)
+                } else {
+                    dataSource.hidePasskey(key)
+                }
             }
+            return true
+        } catch (e: Exception) {
+            e.printStackTrace()
+            return false
         }
     }
 
-    private fun handleCurrentUserDetailRequest(requestJson: String) = runBlocking {
-        val request = JSONObject(requestJson)
-        val userId = request.getString(USER_ID)
-        val updatedName = request.getString(NAME)
-        val updatedDisplayName = request.getString(DISPLAY_NAME)
-        val listPasskeys = dataSource.getPasskeyForUser(userId) ?: emptyList()
-        // Update user details for each passkey
-        for (key in listPasskeys) {
-            val newPasskeyItem = key.copy(username = updatedName, displayName = updatedDisplayName)
-            dataSource.updatePasskey(newPasskeyItem)
+    /**
+     * Handles a {@link SignalCurrentUserDetailsRequest} by updating the username and display name
+     * for all passkeys associated with a given user ID.
+     *
+     * This is useful when the user's profile information (like name or display name)
+     * changes elsewhere, and the credential provider needs to reflect these changes
+     * in its stored passkey data.
+     *
+     * @param requestJson The JSON string payload from the {@link SignalCurrentUserDetailsRequest}.
+     *                   Expected to contain {@code USER_ID}, {@code NAME}, and {@code DISPLAY_NAME}.
+     */
+    private suspend fun handleCurrentUserDetailRequest(requestJson: String): Boolean {
+        try {
+            val request = JSONObject(requestJson)
+            val userId = request.getString(USER_ID)
+            val updatedName = request.getString(NAME)
+            val updatedDisplayName = request.getString(DISPLAY_NAME)
+            val listPasskeys = dataSource.getAllPasskeysForUser(userId) ?: emptyList()
+            // Update user details for each passkey
+            for (key in listPasskeys) {
+                val newPasskeyItem =
+                    key.copy(username = updatedName, displayName = updatedDisplayName)
+                dataSource.updatePasskey(newPasskeyItem)
+            }
+            return true
+        } catch (e: Exception) {
+            e.printStackTrace()
+            return false
         }
     }
 }
diff --git a/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/data/CredentialsDataSource.kt b/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/data/CredentialsDataSource.kt
@@ -127,8 +127,8 @@ class CredentialsDataSource(
         return myVaultDao.getPasskey(credId)
     }
 
-    fun getPasskeyForUser(userId: String): List<PasskeyItem>? {
-        return myVaultDao.getPasskeysForUser(userId)
+    fun getAllPasskeysForUser(userId: String): List<PasskeyItem>? {
+        return myVaultDao.getAllPasskeysForUser(userId)
     }
 
     suspend fun hidePasskey(passkey: PasskeyItem) {
diff --git a/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/data/CredentialsRepository.kt b/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/data/CredentialsRepository.kt
@@ -228,8 +228,9 @@ class CredentialsRepository(
             val credentials = credentialsDataSource.credentialsForSite(request.rpId) ?: return false
 
             val passkeys = credentials.passkeys
-            for (passkey in passkeys) {
-                if (!passkey.hidden) {
+            passkeys
+                .filter { !it.hidden }
+                .forEach { passkey ->
                     val data = Bundle()
                     data.putString("requestJson", option.requestJson)
                     data.putString("credId", passkey.credId)
@@ -259,7 +260,6 @@ class CredentialsRepository(
                     // Add the entry to the response builder.
                     responseBuilder.addCredentialEntry(entry)
                 }
-            }
         } catch (e: IOException) {
             return false
         }
diff --git a/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/data/room/MyVaultDatabase.kt b/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/data/room/MyVaultDatabase.kt
@@ -89,6 +89,6 @@ interface MyVaultDao {
     @Query("SELECT * from passkeys WHERE credId = :credId")
     fun getPasskey(credId: String): PasskeyItem?
 
-    @Query("SELECT * from passkeys WHERE uid = :userId and hidden = false")
-    fun getPasskeysForUser(userId: String): List<PasskeyItem>?
+    @Query("SELECT * from passkeys WHERE uid = :userId")
+    fun getAllPasskeysForUser(userId: String): List<PasskeyItem>?
 }
diff --git a/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/ui/MainActivity.kt b/CredentialProvider/MyVault/app/src/main/java/com/example/android/authentication/myvault/ui/MainActivity.kt
@@ -20,6 +20,7 @@ import androidx.activity.ComponentActivity
 import androidx.activity.compose.setContent
 import androidx.activity.enableEdgeToEdge
 import androidx.core.view.WindowCompat
+import com.example.android.authentication.myvault.R
 import com.example.android.authentication.myvault.createNotificationChannel
 import com.example.android.authentication.myvault.ui.theme.MyVaultTheme
 
@@ -28,8 +29,8 @@ class MainActivity : ComponentActivity() {
         enableEdgeToEdge()
         super.onCreate(savedInstanceState)
         createNotificationChannel(
-            "Signal API notification channel",
-            "Notification channel used for testing Signal APIs. Apps pushes a notification if a Signal from RP is received"
+            getString(R.string.notification_channel_name),
+            getString(R.string.notification_channel_description),
         )
 
         WindowCompat.setDecorFitsSystemWindows(window, false)
diff --git a/CredentialProvider/MyVault/app/src/main/res/values/strings.xml b/CredentialProvider/MyVault/app/src/main/res/values/strings.xml
@@ -75,4 +75,6 @@
     <string name="all_accepted_signal_message">The provider received the AcceptedCredentialIds request and thus updated the list of user credentials</string>
     <string name="user_details_updation">User details updated successfully</string>
     <string name="current_user_signal_message">The provider received the CurrentUserDetails request and thus updated the user details in the credential</string>
+    <string name="notification_channel_description">Notification channel used for testing Signal APIs. Apps pushes a notification if a Signal from RP is received</string>
+    <string name="notification_channel_name">Signal API notification channel</string>
 </resources>
diff --git a/CredentialProvider/MyVault/gradle/libs.versions.toml b/CredentialProvider/MyVault/gradle/libs.versions.toml

Original file line number	Diff line number	Diff line change
`@@ -127,8 +127,8 @@ class CredentialsDataSource(`
`127`	`127`	`return myVaultDao.getPasskey(credId)`
`128`	`128`	`}`
`129`	`129`
`130`		`- fun getPasskeyForUser(userId: String): List<PasskeyItem>? {`
`131`		`- return myVaultDao.getPasskeysForUser(userId)`
	`130`	`+ fun getAllPasskeysForUser(userId: String): List<PasskeyItem>? {`
	`131`	`+ return myVaultDao.getAllPasskeysForUser(userId)`
`132`	`132`	`}`
`133`	`133`
`134`	`134`	`suspend fun hidePasskey(passkey: PasskeyItem) {`
Original file line number	Diff line number	Diff line change
`@@ -89,6 +89,6 @@ interface MyVaultDao {`
`89`	`89`	`@Query("SELECT * from passkeys WHERE credId = :credId")`
`90`	`90`	`fun getPasskey(credId: String): PasskeyItem?`
`91`	`91`
`92`		`- @Query("SELECT * from passkeys WHERE uid = :userId and hidden = false")`
`93`		`- fun getPasskeysForUser(userId: String): List<PasskeyItem>?`
	`92`	`+ @Query("SELECT * from passkeys WHERE uid = :userId")`
	`93`	`+ fun getAllPasskeysForUser(userId: String): List<PasskeyItem>?`
`94`	`94`	`}`