Add Signal API implementation (RP) #161

neelanshsahai · 2025-08-28T10:11:12Z

No description provided.

gemini-code-assist

Summary of Changes

Hello @neelanshsahai, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the Shrine Android application by integrating the Credential Manager Signal API. This integration allows the app to communicate important credential lifecycle events and user metadata to the credential provider, improving overall authentication flow and user experience. Key changes include the addition of new signaling functions, a dedicated 'Update Profile' screen to showcase user detail signaling, and the persistence of critical authentication identifiers. The underlying AndroidX Credential library has also been updated to support these new capabilities.

Highlights

Credential Manager Signal API Implementation: The core of this PR is the implementation of the Credential Manager Signal API. This includes new functions in CredentialManagerUtils to signal unknown credentials, accepted credential IDs, and current user details to the credential provider. This functionality is crucial for improving the user experience and security by allowing the app to communicate credential state changes.
New 'Update Profile' Screen: A new 'Update Profile' screen has been added to the application. This screen serves as a demonstration of how the signalUserDetails API can be used to update user metadata (like name and display name) with the credential provider.
Persistence of Authentication Identifiers: The application now persists key authentication identifiers such as the Relying Party ID (rpId), User ID (userId), and Credential ID (credId) using Android's DataStore. This ensures that these critical pieces of information are available across app sessions for consistent credential management and signaling.
Passkey Management UI Enhancements: The Passkey Management screen has been enhanced with new UI capabilities. Users can now select multiple passkeys using checkboxes, and a new button allows signaling accepted credential IDs, providing more granular control over passkey management.
AndroidX Credential Library Upgrade: The AndroidX Credential library has been updated to version 1.6.0-alpha05. This upgrade is essential as it provides the necessary APIs and functionalities for the newly implemented Credential Manager Signal API.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request adds the implementation for the Credential Manager Signal API, which allows the app to inform the credential provider about changes to credentials on the server. The changes include adding new screens and logic to test these APIs. My review focuses on the correctness of the JSON payloads for the Signal API, the user experience of the new UI elements, and general code quality. I've found a critical issue in how the JSON for signaling accepted credential IDs is constructed, and another critical issue where the UI for deleting credentials doesn't actually perform the deletion on the server. There are also several medium to high severity issues related to JSON construction, hardcoded strings, and UI functionality that should be addressed.

Shrine/app/src/main/java/com/authentication/shrine/CredentialManagerUtils.kt

gemini-code-assist · 2025-08-28T10:14:30Z

Shrine/app/src/main/java/com/authentication/shrine/ui/PasskeyManagementScreen.kt

+    val onSignalBtnClicked = {
+        val credentialIdsList = passkeysList
+            .filter { !it.isSelected }
+            .map { it.id }
+        if (credentialIdsList.isNotEmpty()) {
+            viewModel.signalAccepted(credentialIdsList)
        }
    }


The button associated with this action is labeled "Delete selected credentials", which implies that the selected credentials will be deleted from the server. However, this logic only calls viewModel.signalAccepted, which informs the credential provider about the remaining valid credentials but does not perform any deletion on the Relying Party (RP) server.

This is misleading to the user and can be considered a data handling bug, as the credentials they believe are deleted still exist on the server. You should also call a ViewModel function to delete the selected credentials from the server.

Shrine/app/src/main/java/com/authentication/shrine/CredentialManagerUtils.kt

Shrine/app/src/main/java/com/authentication/shrine/ui/MainMenuScreen.kt

Shrine/app/src/main/java/com/authentication/shrine/ui/PasskeyManagementScreen.kt

Shrine/app/src/main/java/com/authentication/shrine/ui/UpdateProfileScreen.kt

gemini-code-assist

Code Review

This pull request adds the Signal API implementation and related UI for updating user profiles and managing passkeys. The changes are extensive, touching on data models, repositories, ViewModels, and UI screens. My review focuses on improving code correctness, maintainability, and robustness. Key feedback includes fixing a bug in manual JSON construction, addressing a logic issue with credential deletion, improving state management in the new UI, and resolving potential memory leaks in the ViewModel. There are also some minor suggestions for using string resources.

Shrine/app/src/main/java/com/authentication/shrine/CredentialManagerUtils.kt

Shrine/app/src/main/java/com/authentication/shrine/ui/PasskeyManagementScreen.kt

Shrine/app/src/main/java/com/authentication/shrine/ui/viewmodel/UpdateProfileViewModel.kt

Shrine/app/src/main/java/com/authentication/shrine/ui/MainMenuScreen.kt

Shrine/app/src/main/java/com/authentication/shrine/ui/PasskeyManagementScreen.kt

Shrine/app/src/main/java/com/authentication/shrine/ui/UpdateProfileScreen.kt

Shrine/app/src/main/java/com/authentication/shrine/CredentialManagerUtils.kt

cy245 · 2025-09-05T19:13:57Z

Shrine/app/src/main/java/com/authentication/shrine/repository/AuthRepository.kt

                prefs[USERNAME] = username
                response.getSessionId()?.also {
                    prefs[SESSION_ID] = it
+                } ?: run {


I removed this logic here and below in an earlier PR, let's not add it back

Please take pull of latest code in this case as thats merged.

How are we dealing with the case when the response is successful but the Session ID is not present?

This would cause the app to crash when it was encountered since you can't edit dataStore within another dataStore lambda. I checked with Eiji the cases where the sessionId should be present.

cy245 · 2025-09-05T19:21:52Z

Shrine/app/src/main/java/com/authentication/shrine/ui/viewmodel/PasskeyManagementViewModel.kt

+        }
+    }
+
+    fun updateItem(index: Int, passkeysList: List<PasskeyCredential>) {


Is this just a UI update? Why are we managing UI state in the viewmodel?

Just a business logic function that gets called on click of a checkbox. Updates the data (list of credentials) that is scoped in the viewmodel

Shrine/app/src/main/java/com/authentication/shrine/ui/UpdateProfileScreen.kt

Shrine/app/src/main/java/com/authentication/shrine/CredentialManagerUtils.kt

niharika2810 · 2025-09-08T04:49:10Z

Shrine/app/src/main/java/com/authentication/shrine/repository/AuthRepository.kt

                prefs[USERNAME] = username
                response.getSessionId()?.also {
                    prefs[SESSION_ID] = it
+                } ?: run {


Please take pull of latest code in this case as thats merged.

Shrine/app/src/main/java/com/authentication/shrine/ui/MainMenuScreen.kt

niharika2810 · 2025-09-08T04:50:24Z

Shrine/app/src/main/java/com/authentication/shrine/ui/PasskeyManagementScreen.kt

+    val onSignalBtnClicked = {
+        val credentialIdsList = passkeysList
+            .filter { !it.isSelected }
+            .map { it.id }
+        if (credentialIdsList.isNotEmpty()) {
+            viewModel.signalAccepted(credentialIdsList)
        }
    }


Shrine/app/src/main/java/com/authentication/shrine/ui/PasskeyManagementScreen.kt

Shrine/app/src/main/java/com/authentication/shrine/ui/UpdateProfileScreen.kt

Shrine/app/src/main/java/com/authentication/shrine/ui/viewmodel/UpdateProfileViewModel.kt

Shrine/app/src/main/java/com/authentication/shrine/CredentialManagerUtils.kt

jbabs1

No additional comments - please address existing comments especially regarding json request construction and resend for re-review. Thanks Neelansh! Great work!

gemini-code-assist bot reviewed Aug 28, 2025

View reviewed changes

gemini-code-assist bot reviewed Sep 4, 2025

View reviewed changes

cy245 requested changes Sep 5, 2025

View reviewed changes

niharika2810 reviewed Sep 8, 2025

View reviewed changes

niharika2810 requested changes Sep 8, 2025

View reviewed changes

Shrine/app/src/main/java/com/authentication/shrine/CredentialManagerUtils.kt Show resolved Hide resolved

cy245 requested a review from jbabs1 September 11, 2025 17:11

jbabs1 reviewed Sep 11, 2025

View reviewed changes

Neelansh Sahai and others added 2 commits September 19, 2025 00:27

Add Signal API implementation (RP)

a9b7040

Add spotless fixes

4c67e30

neelanshsahai force-pushed the signal_api_rp branch from b12d692 to d1d630c Compare September 18, 2025 19:01

Fix code review changes

e93bf42

neelanshsahai force-pushed the signal_api_rp branch from d1d630c to e93bf42 Compare September 18, 2025 19:08

neelanshsahai requested a review from cy245 September 18, 2025 19:08

Add Signal API implementation (RP) #161

Are you sure you want to change the base?

Add Signal API implementation (RP) #161

Uh oh!

Conversation

neelanshsahai commented Aug 28, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbabs1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!