From cbac773c5ecbc169c92bd5ae60f3e6b0a4922f5d Mon Sep 17 00:00:00 2001 From: Helen Qin Date: Wed, 8 Feb 2023 08:53:57 +0000 Subject: [PATCH] UI bug fixes. 1. As a temporary solution while jetpack isn't ready, use the agreed constant to directly pull the authentication title values. 2. Don't show the "more option" button when there's nothing to show. 3. Updated the test data based on new jetpack updates. 4. Removed/fixed some TODOs. Bug: 268297826 Fix: 268208819 Test: manual Change-Id: I28fa57d6db26406981ad99384e079287dd1af557 --- .../CredentialManager/AndroidManifest.xml | 1 - .../CredentialManagerRepo.kt | 29 +-- .../CredentialSelectorActivity.kt | 13 +- .../CredentialSelectorViewModel.kt | 4 +- .../credentialmanager/DataConverter.kt | 43 ++++- .../android/credentialmanager/TestUtils.kt | 182 ++---------------- .../createflow/CreateCredentialComponents.kt | 19 +- .../getflow/GetCredentialComponents.kt | 2 +- 8 files changed, 92 insertions(+), 201 deletions(-) diff --git a/packages/CredentialManager/AndroidManifest.xml b/packages/CredentialManager/AndroidManifest.xml index 5a4d256d95de4..499d1303e1cf8 100644 --- a/packages/CredentialManager/AndroidManifest.xml +++ b/packages/CredentialManager/AndroidManifest.xml @@ -32,7 +32,6 @@ android:supportsRtl="true" android:theme="@style/Theme.CredentialSelector"> - { - // TODO: fail gracefully throw IllegalStateException("Unrecognized request type: ${requestInfo.type}") } } @@ -167,9 +171,9 @@ class CredentialManagerRepo( ) } + // IMPORTANT: new invocation should be mindful that this method can throw. private fun getCredentialInitialUiState(): GetCredentialUiState? { val providerEnabledList = GetFlowUtils.toProviderList( - // TODO: handle runtime cast error providerEnabledList as List, context ) val requestDisplayInfo = GetFlowUtils.toRequestDisplayInfo(requestInfo, context) @@ -179,9 +183,9 @@ class CredentialManagerRepo( ) } + // IMPORTANT: new invocation should be mindful that this method can throw. private fun getCreateProviderEnableListInitialUiState(): List { val providerEnabledList = CreateFlowUtils.toEnabledProviderList( - // Handle runtime cast error providerEnabledList as List, context ) return providerEnabledList @@ -266,7 +270,7 @@ class CredentialManagerRepo( return listOf( GetCredentialProviderData.Builder("io.enpass.app") .setCredentialEntries( - listOf( + listOf( GetTestUtils.newPasswordEntry( context, "key1", "subkey-1", "elisa.family@outlook.com", null, Instant.ofEpochSecond(8000L) @@ -285,9 +289,12 @@ class CredentialManagerRepo( ), ) ).setAuthenticationEntries( - listOf( - GetTestUtils.newAuthenticationEntry(context, "key2", "subkey-1"), - ) + listOf( + GetTestUtils.newAuthenticationEntry( + context, "key2", "subkey-1", "locked-user1@gmail.com"), + GetTestUtils.newAuthenticationEntry( + context, "key2", "subkey-2", "locked-user2@gmail.com"), + ) ).setActionChips( listOf( GetTestUtils.newActionEntry( @@ -315,9 +322,8 @@ class CredentialManagerRepo( ), ) ).setAuthenticationEntries( - listOf( - GetTestUtils.newAuthenticationEntry(context, "key2", "subkey-1"), - ) + listOf(GetTestUtils.newAuthenticationEntry( + context, "key2", "subkey-1", "foo@email.com")) ).setActionChips( listOf( GetTestUtils.newActionEntry( @@ -388,7 +394,6 @@ class CredentialManagerRepo( CreateCredentialRequest( "androidx.credentials.TYPE_PUBLIC_KEY_CREDENTIAL", credentialData, - // TODO: populate with actual data /*candidateQueryData=*/ Bundle(), /*isSystemProviderRequired=*/ false ), diff --git a/packages/CredentialManager/src/com/android/credentialmanager/CredentialSelectorActivity.kt b/packages/CredentialManager/src/com/android/credentialmanager/CredentialSelectorActivity.kt index 5136f04c3e120..bf69ef4f5b7a8 100644 --- a/packages/CredentialManager/src/com/android/credentialmanager/CredentialSelectorActivity.kt +++ b/packages/CredentialManager/src/com/android/credentialmanager/CredentialSelectorActivity.kt @@ -92,13 +92,20 @@ class CredentialSelectorActivity : ComponentActivity() { handleDialogState(viewModel.uiState.dialogState) } - if (viewModel.uiState.createCredentialUiState != null) { + val createCredentialUiState = viewModel.uiState.createCredentialUiState + val getCredentialUiState = viewModel.uiState.getCredentialUiState + if (createCredentialUiState != null) { CreateCredentialScreen( viewModel = viewModel, + createCredentialUiState = createCredentialUiState, + providerActivityLauncher = launcher + ) + } else if (getCredentialUiState != null) { + GetCredentialScreen( + viewModel = viewModel, + getCredentialUiState = getCredentialUiState, providerActivityLauncher = launcher ) - } else if (viewModel.uiState.getCredentialUiState != null) { - GetCredentialScreen(viewModel = viewModel, providerActivityLauncher = launcher) } else { Log.d(Constants.LOG_TAG, "UI wasn't able to render neither get nor create flow") reportInstantiationErrorAndFinishActivity(credManRepo) diff --git a/packages/CredentialManager/src/com/android/credentialmanager/CredentialSelectorViewModel.kt b/packages/CredentialManager/src/com/android/credentialmanager/CredentialSelectorViewModel.kt index 6bf1513631cec..30b4b86e8f1c9 100644 --- a/packages/CredentialManager/src/com/android/credentialmanager/CredentialSelectorViewModel.kt +++ b/packages/CredentialManager/src/com/android/credentialmanager/CredentialSelectorViewModel.kt @@ -255,14 +255,14 @@ class CredentialSelectorViewModel( } fun createFlowOnEntrySelectedFromFirstUseScreen(activeEntry: ActiveEntry) { + val providerId = activeEntry.activeProvider.id + createFlowOnDefaultChanged(providerId) uiState = uiState.copy( createCredentialUiState = uiState.createCredentialUiState?.copy( currentScreenState = CreateScreenState.CREATION_OPTION_SELECTION, activeEntry = activeEntry ) ) - val providerId = uiState.createCredentialUiState?.activeEntry?.activeProvider?.id - createFlowOnDefaultChanged(providerId) } fun createFlowOnDisabledProvidersSelected() { diff --git a/packages/CredentialManager/src/com/android/credentialmanager/DataConverter.kt b/packages/CredentialManager/src/com/android/credentialmanager/DataConverter.kt index 167b956e92920..ae87d9576f087 100644 --- a/packages/CredentialManager/src/com/android/credentialmanager/DataConverter.kt +++ b/packages/CredentialManager/src/com/android/credentialmanager/DataConverter.kt @@ -17,6 +17,7 @@ package com.android.credentialmanager import android.app.slice.Slice +import android.app.slice.SliceItem import android.content.ComponentName import android.content.Context import android.content.pm.PackageManager @@ -176,7 +177,9 @@ class GetFlowUtils { } - /* From service data structure to UI credential entry list representation. */ + /** + * Note: caller required handle empty list due to parsing error. + */ private fun getCredentialOptionInfoList( providerId: String, credentialEntries: List, @@ -255,6 +258,9 @@ class GetFlowUtils { } } + /** + * Note: caller required handle empty list due to parsing error. + */ private fun getAuthenticationEntryList( providerId: String, providerDisplayName: String, @@ -262,16 +268,24 @@ class GetFlowUtils { authEntryList: List, ): List { val result: MutableList = mutableListOf() - authEntryList.forEach { + authEntryList.forEach { entry -> val structuredAuthEntry = - AuthenticationAction.fromSlice(it.slice) ?: return@forEach + AuthenticationAction.fromSlice(entry.slice) ?: return@forEach + + // TODO: replace with official jetpack code. + val titleItem: SliceItem? = entry.slice.items.firstOrNull { + it.hasHint( + "androidx.credentials.provider.authenticationAction.SLICE_HINT_TITLE") + } + val title: String = titleItem?.text?.toString() ?: providerDisplayName + result.add(AuthenticationEntryInfo( providerId = providerId, - entryKey = it.key, - entrySubkey = it.subkey, + entryKey = entry.key, + entrySubkey = entry.subkey, pendingIntent = structuredAuthEntry.pendingIntent, - fillInIntent = it.frameworkExtrasIntent, - title = providerDisplayName, + fillInIntent = entry.frameworkExtrasIntent, + title = title, icon = providerIcon, )) } @@ -279,7 +293,6 @@ class GetFlowUtils { } private fun getRemoteEntry(providerId: String, remoteEntry: Entry?): RemoteEntryInfo? { - // TODO: should also call fromSlice after getting the official jetpack code. if (remoteEntry == null) { return null } @@ -294,6 +307,9 @@ class GetFlowUtils { ) } + /** + * Note: caller required handle empty list due to parsing error. + */ private fun getActionEntryList( providerId: String, actionEntries: List, @@ -321,7 +337,9 @@ class GetFlowUtils { class CreateFlowUtils { companion object { - // Returns the list (potentially empty) of enabled provider. + /** + * Note: caller required handle empty list due to parsing error. + */ fun toEnabledProviderList( providerDataList: List, context: Context, @@ -346,7 +364,9 @@ class CreateFlowUtils { return providerList } - // Returns the list (potentially empty) of disabled provider. + /** + * Note: caller required handle empty list due to parsing error. + */ fun toDisabledProviderList( providerDataList: List?, context: Context, @@ -532,6 +552,9 @@ class CreateFlowUtils { } else null } + /** + * Note: caller required handle empty list due to parsing error. + */ private fun toCreationOptionInfoList( providerId: String, creationEntries: List, diff --git a/packages/CredentialManager/src/com/android/credentialmanager/TestUtils.kt b/packages/CredentialManager/src/com/android/credentialmanager/TestUtils.kt index e3bbaebfa16fd..a580c6cbf2afd 100644 --- a/packages/CredentialManager/src/com/android/credentialmanager/TestUtils.kt +++ b/packages/CredentialManager/src/com/android/credentialmanager/TestUtils.kt @@ -23,10 +23,11 @@ import android.content.Context import android.content.Intent import android.credentials.Credential.TYPE_PASSWORD_CREDENTIAL import android.credentials.ui.Entry -import android.graphics.drawable.Icon import android.net.Uri import android.provider.Settings import androidx.credentials.provider.CreateEntry +import androidx.credentials.provider.PasswordCredentialEntry +import androidx.credentials.provider.PublicKeyCredentialEntry import java.time.Instant @@ -37,6 +38,7 @@ class GetTestUtils { context: Context, key: String, subkey: String, + title: String, ): Entry { val slice = Slice.Builder( Uri.EMPTY, SliceSpec("AuthenticationAction", 0) @@ -52,6 +54,11 @@ class GetTestUtils { .build(), /*subType=*/null ) + slice.addText( + title, + null, + listOf("androidx.credentials.provider.authenticationAction.SLICE_HINT_TITLE") + ) return Entry( key, subkey, @@ -94,23 +101,6 @@ class GetTestUtils { ) } - private const val SLICE_HINT_TYPE_DISPLAY_NAME = - "androidx.credentials.provider.passwordCredentialEntry.SLICE_HINT_TYPE_DISPLAY_NAME" - private const val SLICE_HINT_TITLE = - "androidx.credentials.provider.passwordCredentialEntry.SLICE_HINT_USER_NAME" - private const val SLICE_HINT_SUBTITLE = - "androidx.credentials.provider.passwordCredentialEntry.SLICE_HINT_TYPE_DISPLAY_NAME" - private const val SLICE_HINT_LAST_USED_TIME_MILLIS = - "androidx.credentials.provider.passwordCredentialEntry.SLICE_HINT_LAST_USED_TIME_MILLIS" - private const val SLICE_HINT_ICON = - "androidx.credentials.provider.passwordCredentialEntry.SLICE_HINT_PROFILE_ICON" - private const val SLICE_HINT_PENDING_INTENT = - "androidx.credentials.provider.passwordCredentialEntry.SLICE_HINT_PENDING_INTENT" - private const val SLICE_HINT_AUTO_ALLOWED = - "androidx.credentials.provider.passwordCredentialEntry.SLICE_HINT_AUTO_ALLOWED" - private const val AUTO_SELECT_TRUE_STRING = "true" - private const val AUTO_SELECT_FALSE_STRING = "false" - internal fun newPasswordEntry( context: Context, key: String, @@ -125,90 +115,13 @@ class GetTestUtils { val pendingIntent = PendingIntent.getActivity( context, 1, intent, (PendingIntent.FLAG_MUTABLE or PendingIntent.FLAG_UPDATE_CURRENT - or PendingIntent.FLAG_ONE_SHOT) - ) - return Entry( - key, - subkey, - toPasswordSlice(userName, userDisplayName, pendingIntent, lastUsedTime), - Intent() + or PendingIntent.FLAG_ONE_SHOT) ) + val passwordEntry = PasswordCredentialEntry.Builder(context, userName, pendingIntent) + .setDisplayName(userDisplayName).setLastUsedTime(lastUsedTime).build() + return Entry(key, subkey, passwordEntry.slice, Intent()) } - private fun toPasswordSlice( - title: CharSequence, - subTitle: CharSequence?, - pendingIntent: PendingIntent, - lastUsedTime: Instant?, - icon: Icon? = null, - isAutoSelectAllowed: Boolean = true - ): Slice { - val type = TYPE_PASSWORD_CREDENTIAL - val autoSelectAllowed = if (isAutoSelectAllowed) { - AUTO_SELECT_TRUE_STRING - } else { - AUTO_SELECT_FALSE_STRING - } - val sliceBuilder = Slice.Builder( - Uri.EMPTY, SliceSpec( - type, 1 - ) - ) - .addText( - "Password", /*subType=*/null, - listOf(SLICE_HINT_TYPE_DISPLAY_NAME) - ) - .addText( - title, /*subType=*/null, - listOf(SLICE_HINT_TITLE) - ) - .addText( - subTitle, /*subType=*/null, - listOf(SLICE_HINT_SUBTITLE) - ) - .addText( - autoSelectAllowed, /*subType=*/null, - listOf(SLICE_HINT_AUTO_ALLOWED) - ) - if (lastUsedTime != null) { - sliceBuilder.addLong( - lastUsedTime.toEpochMilli(), - /*subType=*/null, - listOf(SLICE_HINT_LAST_USED_TIME_MILLIS) - ) - } - if (icon != null) { - sliceBuilder.addIcon( - icon, /*subType=*/null, - listOf(SLICE_HINT_ICON) - ) - } - sliceBuilder.addAction( - pendingIntent, - Slice.Builder(sliceBuilder) - .addHints(listOf(SLICE_HINT_PENDING_INTENT)) - .build(), - /*subType=*/null - ) - return sliceBuilder.build() - } - - - private const val PASSKEY_SLICE_HINT_TYPE_DISPLAY_NAME = - "androidx.credentials.provider.publicKeyCredEntry.SLICE_HINT_TYPE_DISPLAY_NAME" - private const val PASSKEY_SLICE_HINT_TITLE = - "androidx.credentials.provider.publicKeyCredEntry.SLICE_HINT_USER_NAME" - private const val PASSKEY_SLICE_HINT_SUBTITLE = - "androidx.credentials.provider.publicKeyCredEntry.SLICE_HINT_TYPE_DISPLAY_NAME" - private const val PASSKEY_SLICE_HINT_LAST_USED_TIME_MILLIS = - "androidx.credentials.provider.publicKeyCredEntry.SLICE_HINT_LAST_USED_TIME_MILLIS" - private const val PASSKEY_SLICE_HINT_ICON = - "androidx.credentials.provider.publicKeyCredEntry.SLICE_HINT_PROFILE_ICON" - private const val PASSKEY_SLICE_HINT_PENDING_INTENT = - "androidx.credentials.provider.publicKeyCredEntry.SLICE_HINT_PENDING_INTENT" - private const val PASSKEY_SLICE_HINT_AUTO_ALLOWED = - "androidx.credentials.provider.publicKeyCredEntry.SLICE_HINT_AUTO_ALLOWED" - internal fun newPasskeyEntry( context: Context, key: String, @@ -223,72 +136,11 @@ class GetTestUtils { val pendingIntent = PendingIntent.getActivity( context, 1, intent, (PendingIntent.FLAG_MUTABLE or PendingIntent.FLAG_UPDATE_CURRENT - or PendingIntent.FLAG_ONE_SHOT) + or PendingIntent.FLAG_ONE_SHOT) ) - return Entry( - key, subkey, toPasskeySlice( - userName, userDisplayName, pendingIntent, lastUsedTime - ), - Intent() - ) - } - - private fun toPasskeySlice( - title: CharSequence, - subTitle: CharSequence?, - pendingIntent: PendingIntent, - lastUsedTime: Instant?, - icon: Icon? = null, - isAutoSelectAllowed: Boolean = true - ): Slice { - val type = "androidx.credentials.TYPE_PUBLIC_KEY_CREDENTIAL" - val autoSelectAllowed = if (isAutoSelectAllowed) { - AUTO_SELECT_TRUE_STRING - } else { - AUTO_SELECT_FALSE_STRING - } - val sliceBuilder = Slice.Builder( - Uri.EMPTY, SliceSpec( - type, 1 - ) - ) - .addText( - "Passkey", /*subType=*/null, - listOf(PASSKEY_SLICE_HINT_TYPE_DISPLAY_NAME) - ) - .addText( - title, /*subType=*/null, - listOf(PASSKEY_SLICE_HINT_TITLE) - ) - .addText( - subTitle, /*subType=*/null, - listOf(PASSKEY_SLICE_HINT_SUBTITLE) - ) - .addText( - autoSelectAllowed, /*subType=*/null, - listOf(PASSKEY_SLICE_HINT_AUTO_ALLOWED) - ) - if (lastUsedTime != null) { - sliceBuilder.addLong( - lastUsedTime.toEpochMilli(), - /*subType=*/null, - listOf(PASSKEY_SLICE_HINT_LAST_USED_TIME_MILLIS) - ) - } - if (icon != null) { - sliceBuilder.addIcon( - icon, /*subType=*/null, - listOf(PASSKEY_SLICE_HINT_ICON) - ) - } - sliceBuilder.addAction( - pendingIntent, - Slice.Builder(sliceBuilder) - .addHints(listOf(PASSKEY_SLICE_HINT_PENDING_INTENT)) - .build(), - /*subType=*/null - ) - return sliceBuilder.build() + val passkeyEntry = PublicKeyCredentialEntry.Builder(context, userName, pendingIntent) + .setDisplayName(userDisplayName).setLastUsedTime(lastUsedTime).build() + return Entry(key, subkey, passkeyEntry.slice, Intent()) } } } @@ -326,7 +178,7 @@ class CreateTestUtils { val pendingIntent = PendingIntent.getActivity( context, 1, intent, (PendingIntent.FLAG_MUTABLE or PendingIntent.FLAG_UPDATE_CURRENT - or PendingIntent.FLAG_ONE_SHOT) + or PendingIntent.FLAG_ONE_SHOT) ) val credCountMap = mutableMapOf() passwordCount?.let { credCountMap.put(TYPE_PASSWORD_CREDENTIAL, it) } diff --git a/packages/CredentialManager/src/com/android/credentialmanager/createflow/CreateCredentialComponents.kt b/packages/CredentialManager/src/com/android/credentialmanager/createflow/CreateCredentialComponents.kt index adb54676223b3..558c229743c86 100644 --- a/packages/CredentialManager/src/com/android/credentialmanager/createflow/CreateCredentialComponents.kt +++ b/packages/CredentialManager/src/com/android/credentialmanager/createflow/CreateCredentialComponents.kt @@ -62,9 +62,9 @@ import com.android.credentialmanager.ui.theme.LocalAndroidColorScheme @Composable fun CreateCredentialScreen( viewModel: CredentialSelectorViewModel, + createCredentialUiState: CreateCredentialUiState, providerActivityLauncher: ManagedActivityResultLauncher ) { - val createCredentialUiState = viewModel.uiState.createCredentialUiState ?: return ModalBottomSheet( sheetContent = { // Hide the sheet content as opposed to the whole bottom sheet to maintain the scrim @@ -94,6 +94,7 @@ fun CreateCredentialScreen( requestDisplayInfo = createCredentialUiState.requestDisplayInfo, enabledProviderList = createCredentialUiState.enabledProviders, providerInfo = createCredentialUiState.activeEntry?.activeProvider!!, + hasDefaultProvider = createCredentialUiState.hasDefaultProvider, createOptionInfo = createCredentialUiState.activeEntry.activeEntryInfo as CreateOptionInfo, @@ -264,7 +265,6 @@ fun ConfirmationCard( } } -@OptIn(ExperimentalMaterial3Api::class) @Composable fun ProviderSelectionCard( requestDisplayInfo: RequestDisplayInfo, @@ -351,7 +351,6 @@ fun ProviderSelectionCard( thickness = 24.dp, color = Color.Transparent ) - // TODO: handle the error situation that if multiple remoteInfos exists enabledProviderList.forEach { enabledProvider -> if (enabledProvider.remoteEntry != null) { Row( @@ -363,6 +362,7 @@ fun ProviderSelectionCard( onMoreOptionsSelected ) } + return@forEach } } Divider( @@ -463,7 +463,6 @@ fun MoreOptionsSelectionCard( ) } } - // TODO: handle the error situation that if multiple remoteInfos exists enabledProviderList.forEach { if (it.remoteEntry != null) { item { @@ -472,6 +471,7 @@ fun MoreOptionsSelectionCard( onRemoteEntrySelected = onRemoteEntrySelected, ) } + return@forEach } } } @@ -549,6 +549,7 @@ fun CreationSelectionCard( onOptionSelected: (BaseEntry) -> Unit, onConfirm: () -> Unit, onMoreOptionsSelected: () -> Unit, + hasDefaultProvider: Boolean, ) { ContainerCard() { Column() { @@ -601,7 +602,6 @@ fun CreationSelectionCard( onOptionSelected = onOptionSelected ) } - var shouldShowMoreOptionsButton = false var createOptionsSize = 0 var remoteEntry: RemoteInfo? = null enabledProviderList.forEach { enabledProvider -> @@ -610,8 +610,13 @@ fun CreationSelectionCard( } createOptionsSize += enabledProvider.createOptions.size } - if (createOptionsSize > 1 || remoteEntry != null) { - shouldShowMoreOptionsButton = true + val shouldShowMoreOptionsButton = if (!hasDefaultProvider) { + // User has already been presented with all options on the default provider + // selection screen. Don't show them again. Therefore, only show the more option + // button if remote option is present. + remoteEntry != null + } else { + createOptionsSize > 1 || remoteEntry != null } Row( horizontalArrangement = diff --git a/packages/CredentialManager/src/com/android/credentialmanager/getflow/GetCredentialComponents.kt b/packages/CredentialManager/src/com/android/credentialmanager/getflow/GetCredentialComponents.kt index 8b311fe23914d..48ee287f8672d 100644 --- a/packages/CredentialManager/src/com/android/credentialmanager/getflow/GetCredentialComponents.kt +++ b/packages/CredentialManager/src/com/android/credentialmanager/getflow/GetCredentialComponents.kt @@ -76,9 +76,9 @@ import com.android.credentialmanager.ui.theme.LocalAndroidColorScheme @Composable fun GetCredentialScreen( viewModel: CredentialSelectorViewModel, + getCredentialUiState: GetCredentialUiState, providerActivityLauncher: ManagedActivityResultLauncher ) { - val getCredentialUiState = viewModel.uiState.getCredentialUiState ?: return if (getCredentialUiState.currentScreenState != GetScreenState.REMOTE_ONLY) { ModalBottomSheet( sheetContent = {