android / identity-samples

Multiple samples showing the best practices in identity on Android.
Apache License 2.0
322 stars 198 forks source link

Use application context for CredentialManager #77

Closed yschimke closed 1 month ago

yschimke commented 2 months ago

I'm trying to use the CredentialManager in a Hilt + Compose app. I was struggling with how to have an Activity when creating this in Hilt, but I think that isn't actually a requirement, at least from

https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:credentials/credentials/src/main/java/androidx/credentials/CredentialManager.kt;l=87?q=CredentialManager

I don't want to refactor this sample more, but this should allow it to be pulled up to the Application if desired

yschimke commented 2 months ago

Using this to kick off discussion, if I'm wrong, please close, but then docs should be updated.

yschimke commented 2 months ago

This is a good example

https://github.com/Dashlane/android-passkey-example/blob/9147435939d460f7e39f6aa2d0f6e52ae8658665/app/src/main/java/com/dashlane/dashlanepasskeydemo/injection/DashlanePasskeyModule.kt#L5

niharika2810 commented 1 month ago

Hi We cant use application context as this will cause memory leak. We need activity context to show dialog above that activity. Closing this PR, let us know if you have any questions.

yschimke commented 1 month ago

Why would application context leak memory? The activity context is usually the thing that must be avoided leaking.

This sample https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:credentials/credentials/samples/src/main/java/androidx/credentials/samples/CredentialManagerSample.kt;l=48?q=credentialmanager%20viewmodel%20f:androidx seems to agree

/** Sample showing how to invoke getCredential API. */
fun callGetCredential(
    context: Context,
    activity: Activity,
    signInWithCredential: (Credential) -> Unit,
    handleGetCredentialFailure: (GetCredentialException) -> Unit,
) {
    val credentialManager = CredentialManager.create(context)
    ...
    // It will be canceled if this coroutine scope is canceled. If you want the operation to persist
    // through your UI lifecycle (e.g. configuration changes), choose a coroutine scope that is
    // broader than your UI lifecycle (e.g. ViewModelScope)
    yourCoroutineScope.launch {
        try {
            val response = credentialManager.getCredential(
                // Important: use an Activity context to ensure that the system credential selector
                // ui is launched within the same activity stack to avoid undefined UI transition
                // behavior.
                context = activity,
                request = request,
            )

If you use activity context, then you can't hoist this to a ViewModelScope as it will definitely leak.

yschimke commented 1 month ago

I'd suggest creating a CredentialManager with an activity context should be an anti-pattern, but it's not clear the API is correct at all. It's hard to reason about with CredentialManager in a ViewModel scope and getCredential needing an activity context.

You have to trust that the activity context is only used at launch, otherwise it is a leak and probably a crash.