Skip to content

Implement conditional create in Shrine #189

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
cy245 wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Implement conditional create in Shrine #189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion Shrine/app/src/main/java/com/authentication/shrine/CredentialManagerUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,19 +171,24 @@ class CredentialManagerUtils @Inject constructor(
*
* @param requestResult The result of the passkey creation request.
* @param context The activity context from the Composable, to be used in Credential Manager APIs
* @param isConditional Whether the passkey creation is conditional.
* @return The [CreatePublicKeyCredentialResponse] object containing the passkey, or null if an error occurred.
*/
@SuppressLint("PublicKeyCredential")
suspend fun createPasskey(
requestResult: JSONObject,
context: Context,
isConditional: Boolean = false,
): GenericCredentialManagerResponse {
val passkeysEligibility = PasskeysEligibility.isPasskeySupported(context)
if (!passkeysEligibility.isEligible) {
return GenericCredentialManagerResponse.Error(errorMessage = passkeysEligibility.reason)
}

val credentialRequest = CreatePublicKeyCredentialRequest(requestResult.toString())
val credentialRequest = CreatePublicKeyCredentialRequest(
requestJson = requestResult.toString(),
isConditional = isConditional
)
val credentialResponse: CreatePublicKeyCredentialResponse
try {
credentialResponse = credentialManager.createCredential(
Expand Down
35 changes: 6 additions & 29 deletions Shrine/app/src/main/java/com/authentication/shrine/api/AuthApiService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package com.authentication.shrine.api
import com.authentication.shrine.model.EditUsernameRequest
import com.authentication.shrine.model.FederationOptionsRequest
import com.authentication.shrine.model.GenericAuthResponse
import com.authentication.shrine.model.LoginUsernamePasswordRequest
import com.authentication.shrine.model.PasskeysList
import com.authentication.shrine.model.PasswordRequest
import com.authentication.shrine.model.RegisterRequestRequestBody
Expand All @@ -40,41 +41,17 @@ import retrofit2.http.Query
interface AuthApiService {

/**
* Sets or updates the username for the current session.
* Logs in with the username and password
*
* @param username The request body containing the new username.
* @param usernamePasswordRequest The request body containing the username and password.
* @return A Retrofit {@link Response} wrapping a {@link GenericAuthResponse},
* indicating the success or failure of the operation.
*/
@POST("auth/username")
suspend fun setUsername(
@Body username: EditUsernameRequest,
@POST("auth/username-password")
suspend fun loginWithUsernamePassword(
@Body usernamePasswordRequest: LoginUsernamePasswordRequest,
): Response<GenericAuthResponse>

/**
* Sets or updates the password for the authenticated user.
*
* @param cookie The session cookie for authentication.
* @param password The request body containing the new password information.
* @return A Retrofit {@link Response} wrapping a {@link GenericAuthResponse},
* indicating the success or failure of the operation.
*/
@POST("auth/password")
suspend fun setPassword(
@Header("Cookie") cookie: String,
@Body password: PasswordRequest,
): Response<GenericAuthResponse>

/**
* Initiates a WebAuthn registration ceremony by requesting registration options
* from the server.
*
* @param cookie The session cookie for authentication.
* @param requestBody The request body, potentially containing user information
* or relying party details for the registration request.
* @return A Retrofit {@link Response} wrapping a {@link RegisterRequestResponse},
* which contains the challenge and options for the WebAuthn registration.
*/
@POST("webauthn/registerRequest")
suspend fun registerRequest(
@Header("Cookie") cookie: String,
Expand Down
22 changes: 6 additions & 16 deletions Shrine/app/src/main/java/com/authentication/shrine/model/LoginRequest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,15 @@
package com.authentication.shrine.model

/**
* Represents the request body for setting or updating an existing username.
* This data class is typically used for serialization (e.g., with Gson or Kotlinx Serialization)
* when making API calls related to username management.
* Represents the request body for logging in with a username and password.
*
* @property username The username to be set or updated.
* @property username The username to log in with.
* @property password The password to log in with.
*/
data class EditUsernameRequest(

data class LoginUsernamePasswordRequest(
val username: String,
val password: String,
)

/**
Expand All @@ -38,14 +39,3 @@ data class RegisterUsernameRequest(
val username: String,
val displayName: String,
)

/**
* Represents the request body for setting or updating a password.
* This data class is typically used for serialization (e.g., with Gson or Kotlinx Serialization)
* when making API calls related to password management.
*
* @property password The new password.
*/
data class PasswordRequest(
val password: String,
)
43 changes: 2 additions & 41 deletions Shrine/app/src/main/java/com/authentication/shrine/repository/AuthRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import com.authentication.shrine.model.AuthResult
import com.authentication.shrine.model.CredmanResponse
import com.authentication.shrine.model.EditUsernameRequest
import com.authentication.shrine.model.FederationOptionsRequest
import com.authentication.shrine.model.LoginUsernamePasswordRequest
import com.authentication.shrine.model.PasskeysList
import com.authentication.shrine.model.PasswordRequest
import com.authentication.shrine.model.RegisterRequestRequestBody
Expand Down Expand Up @@ -140,7 +141,7 @@ class AuthRepository @Inject constructor(
*/
suspend fun login(username: String, password: String): AuthResult<Unit> {
return try {
val response = authApiService.setUsername(EditUsernameRequest(username = username))
val response = authApiService.loginWithUsernamePassword(LoginUsernamePasswordRequest(username, password))
if (response.isSuccessful) {
dataStore.edit { prefs ->
prefs[USERNAME] = response.body()?.username.orEmpty()
Expand All @@ -149,7 +150,6 @@ class AuthRepository @Inject constructor(
prefs[SESSION_ID] = it
}
}
setSessionWithPassword(password)
AuthResult.Success(Unit)
} else {
if (response.code() == 401) {
Expand All @@ -166,45 +166,6 @@ class AuthRepository @Inject constructor(
}
}

/**
* Signs in with a password.
*
* @param password The password to use.
* @return True if the sign-in was successful, false otherwise.
*/
private suspend fun setSessionWithPassword(password: String): Boolean {
val username = dataStore.read(USERNAME)
val sessionId = dataStore.read(SESSION_ID)
if (!username.isNullOrEmpty() && !sessionId.isNullOrEmpty()) {
try {
val response = authApiService.setPassword(
cookie = sessionId.createCookieHeader(),
password = PasswordRequest(password = password),
)
if (response.isSuccessful) {
dataStore.edit { prefs ->
prefs[USERNAME] = response.body()?.username.orEmpty()
prefs[DISPLAYNAME] = response.body()?.displayName.orEmpty()
response.getSessionId()?.also {
prefs[SESSION_ID] = it
}
}
return true
} else if (response.code() == 401) {
signOut()
}
} catch (e: ApiException) {
Log.e(TAG, "Invalid login credentials", e)

// Remove previously stored credentials and start login over again
signOut()
}
} else {
Log.e(TAG, "Please check if username and session id is present in your datastore")
}
return false
}

/**
* Clears all the sign-in information.
*/
Expand Down
16 changes: 14 additions & 2 deletions Shrine/app/src/main/java/com/authentication/shrine/ui/AuthenticationScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import com.authentication.shrine.ui.common.ShrineLoader
import com.authentication.shrine.ui.theme.ShrineTheme
import com.authentication.shrine.ui.viewmodel.AuthenticationUiState
import com.authentication.shrine.ui.viewmodel.AuthenticationViewModel
import org.json.JSONObject

/**
* Stateful composable function for Authentication screen.
Expand Down Expand Up @@ -86,9 +87,20 @@ fun AuthenticationScreen(

val onSignInWithPasskeyOrPasswordRequest = {
viewModel.signInWithPasskeyOrPasswordRequest(
onSuccess = { flag ->
onSuccess = { isPasswordCredential ->
if (isPasswordCredential) {
viewModel.conditionalCreatePasskey(
createPasskeyOnCredMan = { createPasskeyRequestObj: JSONObject ->
credentialManagerUtils.createPasskey(
requestResult = createPasskeyRequestObj,
context = context,
isConditional = true,
)
}
)
}
createRestoreKey()
navigateToHome(flag)
navigateToHome(!isPasswordCredential)
Comment on lines +91 to +103
Copy link

@gemini-code-assist gemini-code-assist bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

When a user signs in with a password (isPasswordCredential is true), both viewModel.conditionalCreatePasskey() and createRestoreKey() are called. These functions trigger asynchronous credential creation flows that can run in parallel. This could lead to a confusing user experience with multiple system prompts for creating credentials appearing simultaneously, and may also cause race conditions. It would be better to make these calls mutually exclusive. For instance, you could offer to create a standard passkey after a password login, and offer to create a restore key after a passkey login.

                if (isPasswordCredential) {
                    viewModel.conditionalCreatePasskey(
                        createPasskeyOnCredMan = { createPasskeyRequestObj: JSONObject ->
                            credentialManagerUtils.createPasskey(
                                requestResult = createPasskeyRequestObj,
                                context = context,
                                isConditional = true,
                            )
                        }
                    )
                } else {
                    createRestoreKey()
                }
                navigateToHome(!isPasswordCredential)

},
getCredential = { jsonObject ->
credentialManagerUtils.getPasskeyOrPasswordCredential(
Expand Down
26 changes: 26 additions & 0 deletions Shrine/app/src/main/java/com/authentication/shrine/ui/viewmodel/AuthenticationViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,32 @@ class AuthenticationViewModel @Inject constructor(
}
}
}

/**
* Conditionally creates a passkey after a successful password login.
*
* @param createPasskeyOnCredMan A suspend function that takes a [JSONObject] and returns a
* [GenericCredentialManagerResponse]. This function is responsible for creating
* the passkey.
*/
fun conditionalCreatePasskey(
createPasskeyOnCredMan: suspend (createPasskeyRequestObj: JSONObject) -> GenericCredentialManagerResponse,
) {
coroutineScope.launch {
when (val result = repository.registerPasskeyCreationRequest()) {
is AuthResult.Success -> {
val createPasskeyResponse = createPasskeyOnCredMan(result.data)
if (createPasskeyResponse is GenericCredentialManagerResponse.CreatePasskeySuccess) {
repository.registerPasskeyCreationResponse(createPasskeyResponse.createPasskeyResponse)
Copy link

@gemini-code-assist gemini-code-assist bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The result of repository.registerPasskeyCreationResponse() is not being checked. This method returns an AuthResult which can indicate failure. If the server fails to register the passkey, this failure is silently ignored. This could lead to a state where a passkey exists on the device but is not usable because the server is unaware of it. It's important to handle the AuthResult.Failure case to ensure data consistency and aid in debugging.

                        val responseResult = repository.registerPasskeyCreationResponse(createPasskeyResponse.createPasskeyResponse)
                        if (responseResult is AuthResult.Failure) {
                            Log.e(TAG, "Failed to register conditional passkey with server: ${responseResult.error}")
                        }

}
}

is AuthResult.Failure -> {
Log.e(TAG, "Error during conditional passkey creation.")
Copy link

@gemini-code-assist gemini-code-assist bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error message for a failed passkey creation request is quite generic. For better diagnostics and debugging, it would be helpful to include the specific error details from the AuthResult.Failure object in the log message.

                    Log.e(TAG, "Error during conditional passkey creation: ${result.error}")

}
}
}
}
}

/**
Expand Down
Loading