From 7b971f14724ef1b632a4e57a1b2be10b2561e138 Mon Sep 17 00:00:00 2001 From: Adam Velebil Date: Tue, 10 Sep 2024 14:09:18 +0200 Subject: [PATCH] remove nfc retries --- .../yubico/authenticator/AppContextManager.kt | 4 +- .../com/yubico/authenticator/MainActivity.kt | 9 +++- .../authenticator/device/DeviceManager.kt | 48 +++---------------- .../fido/FidoConnectionHelper.kt | 14 +++--- .../yubico/authenticator/fido/FidoManager.kt | 25 +++++++--- .../management/ManagementConnectionHelper.kt | 3 +- .../yubico/authenticator/oath/OathManager.kt | 47 ++++++++++++++---- lib/android/overlay/nfc/nfc_overlay.dart | 7 ++- lib/app/views/main_page.dart | 41 +++++++--------- 9 files changed, 102 insertions(+), 96 deletions(-) diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/AppContextManager.kt b/android/app/src/main/kotlin/com/yubico/authenticator/AppContextManager.kt index e40c6ef0..4467b6ad 100755 --- a/android/app/src/main/kotlin/com/yubico/authenticator/AppContextManager.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/AppContextManager.kt @@ -22,11 +22,13 @@ import com.yubico.yubikit.core.YubiKeyDevice * Provides behavior to run when a YubiKey is inserted/tapped for a specific view of the app. */ abstract class AppContextManager { - abstract suspend fun processYubiKey(device: YubiKeyDevice) + abstract suspend fun processYubiKey(device: YubiKeyDevice): Boolean open fun dispose() {} open fun onPause() {} + + open fun onError() {} } class ContextDisposedException : Exception() \ No newline at end of file diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/MainActivity.kt b/android/app/src/main/kotlin/com/yubico/authenticator/MainActivity.kt index c5daf918..57301c5f 100644 --- a/android/app/src/main/kotlin/com/yubico/authenticator/MainActivity.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/MainActivity.kt @@ -321,6 +321,7 @@ class MainActivity : FlutterFragmentActivity() { appMethodChannel.nfcStateChanged(NfcState.ONGOING) } + deviceManager.scpKeyParams = null // If NFC and FIPS check for SCP11b key if (device.transport == Transport.NFC && deviceInfo.fipsCapable != 0) { logger.debug("Checking for usable SCP11b key...") @@ -340,6 +341,7 @@ class MainActivity : FlutterFragmentActivity() { } } catch (e: Exception) { logger.debug("Exception while getting scp keys: ", e) + contextManager?.onError() if (device is NfcYubiKeyDevice) { appMethodChannel.nfcStateChanged(NfcState.FAILURE) } @@ -373,9 +375,12 @@ class MainActivity : FlutterFragmentActivity() { contextManager?.let { try { - it.processYubiKey(device) - if (!switchedContext && device is NfcYubiKeyDevice) { + val requestHandled = it.processYubiKey(device) + if (requestHandled) { appMethodChannel.nfcStateChanged(NfcState.SUCCESS) + } + if (!switchedContext && device is NfcYubiKeyDevice) { + device.remove { appMethodChannel.nfcStateChanged(NfcState.IDLE) } diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/device/DeviceManager.kt b/android/app/src/main/kotlin/com/yubico/authenticator/device/DeviceManager.kt index 664ac246..8968fd79 100644 --- a/android/app/src/main/kotlin/com/yubico/authenticator/device/DeviceManager.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/device/DeviceManager.kt @@ -32,6 +32,7 @@ import com.yubico.yubikit.core.smartcard.scp.ScpKeyParams import com.yubico.yubikit.management.Capability import kotlinx.coroutines.CancellationException import org.slf4j.LoggerFactory +import java.io.IOException interface DeviceListener { // a USB device is connected @@ -174,7 +175,6 @@ class DeviceManager( fun setDeviceInfo(deviceInfo: Info?) { appViewModel.setDeviceInfo(deviceInfo) - this.scpKeyParams = null } fun isUsbKeyConnected(): Boolean { @@ -189,16 +189,12 @@ class DeviceManager( suspend fun withKey( onUsb: suspend (UsbYubiKeyDevice) -> T, onNfc: suspend () -> com.yubico.yubikit.core.util.Result, - onCancelled: () -> Unit, - retryOnNfcFailure: Boolean + onCancelled: () -> Unit ): T = appViewModel.connectedYubiKey.value?.let { onUsb(it) - } ?: if (retryOnNfcFailure == true) { - onNfcWithRetries(onNfc, onCancelled) - } else { - onNfc(onNfc, onCancelled) - } + } ?: onNfc(onNfc, onCancelled) + private suspend fun onNfc( onNfc: suspend () -> com.yubico.yubikit.core.util.Result, @@ -210,42 +206,12 @@ class DeviceManager( } try { - return onNfc.invoke().value + return onNfc.invoke().value.also { + appMethodChannel.nfcStateChanged(NfcState.SUCCESS) + } } catch (e: Exception) { appMethodChannel.nfcStateChanged(NfcState.FAILURE) throw e } } - - private suspend fun onNfcWithRetries( - onNfc: suspend () -> com.yubico.yubikit.core.util.Result, - onCancelled: () -> Unit - ): T { - - nfcOverlayManager.show { - logger.debug("NFC action with retries was cancelled") - onCancelled.invoke() - } - - while (true) { - try { - return onNfc.invoke().value - } catch (e: Exception) { - - logger.debug("NFC action failed, asking to try again. Failure: ", e) - if (e is CancellationException) { - throw e - } - - if (e is ContextDisposedException) { - // the key does not have the needed context anymore - // we cannot continue - appMethodChannel.nfcStateChanged(NfcState.FAILURE) - throw e - } - - appMethodChannel.nfcStateChanged(NfcState.FAILURE) - } - } - } } \ No newline at end of file diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoConnectionHelper.kt b/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoConnectionHelper.kt index cdb51367..d422706f 100644 --- a/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoConnectionHelper.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoConnectionHelper.kt @@ -30,17 +30,19 @@ import kotlin.coroutines.suspendCoroutine class FidoConnectionHelper(private val deviceManager: DeviceManager) { private var pendingAction: FidoAction? = null - private var deviceInfoTimer: TimerTask? = null - fun invokePending(fidoSession: YubiKitFidoSession) { + fun invokePending(fidoSession: YubiKitFidoSession): Boolean { + var requestHandled = true pendingAction?.let { action -> - action.invoke(Result.success(fidoSession)) pendingAction = null + // it is the pending action who handles this request + requestHandled = false + action.invoke(Result.success(fidoSession)) } + return requestHandled } fun cancelPending() { - deviceInfoTimer?.cancel() pendingAction?.let { action -> action.invoke(Result.failure(CancellationException())) pendingAction = null @@ -49,7 +51,6 @@ class FidoConnectionHelper(private val deviceManager: DeviceManager) { suspend fun useSession( updateDeviceInfo: Boolean = false, - retryOnNfcFailure: Boolean = true, block: (YubiKitFidoSession) -> T ): T { FidoManager.updateDeviceInfo.set(updateDeviceInfo) @@ -59,8 +60,7 @@ class FidoConnectionHelper(private val deviceManager: DeviceManager) { onCancelled = { pendingAction?.invoke(Result.failure(CancellationException())) pendingAction = null - }, - retryOnNfcFailure = retryOnNfcFailure + } ) } diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoManager.kt b/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoManager.kt index 0c43122d..b1d648c1 100644 --- a/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoManager.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoManager.kt @@ -127,8 +127,6 @@ class FidoManager( pinStore ) - - init { pinRetries = null @@ -176,6 +174,12 @@ class FidoManager( } } + override fun onError() { + super.onError() + logger.debug("Cancel any pending action because of upstream error") + connectionHelper.cancelPending() + } + override fun dispose() { super.dispose() deviceManager.removeDeviceListener(this) @@ -186,15 +190,16 @@ class FidoManager( coroutineScope.cancel() } - override suspend fun processYubiKey(device: YubiKeyDevice) { + override suspend fun processYubiKey(device: YubiKeyDevice): Boolean { + var requestHandled = true try { if (device.supportsConnection(FidoConnection::class.java)) { device.withConnection { connection -> - processYubiKey(connection, device) + requestHandled = processYubiKey(connection, device) } } else { device.withConnection { connection -> - processYubiKey(connection, device) + requestHandled = processYubiKey(connection, device) } } @@ -207,10 +212,14 @@ class FidoManager( // Clear any cached FIDO state fidoViewModel.clearSessionState() + throw e } + + return requestHandled } - private fun processYubiKey(connection: YubiKeyConnection, device: YubiKeyDevice) { + private fun processYubiKey(connection: YubiKeyConnection, device: YubiKeyDevice): Boolean { + var requestHandled = true val fidoSession = if (connection is FidoConnection) { YubiKitFidoSession(connection) @@ -229,7 +238,7 @@ class FidoManager( val sameDevice = currentSession == previousSession if (device is NfcYubiKeyDevice && (sameDevice || resetHelper.inProgress)) { - connectionHelper.invokePending(fidoSession) + requestHandled = connectionHelper.invokePending(fidoSession) } else { if (!sameDevice) { @@ -253,6 +262,8 @@ class FidoManager( Session(infoData, pinStore.hasPin(), pinRetries) ) } + + return requestHandled } private fun getPinPermissionsCM(fidoSession: YubiKitFidoSession): Int { diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/management/ManagementConnectionHelper.kt b/android/app/src/main/kotlin/com/yubico/authenticator/management/ManagementConnectionHelper.kt index 5c7a6c30..4bacb524 100644 --- a/android/app/src/main/kotlin/com/yubico/authenticator/management/ManagementConnectionHelper.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/management/ManagementConnectionHelper.kt @@ -39,8 +39,7 @@ class ManagementConnectionHelper( onCancelled = { action?.invoke(Result.failure(CancellationException())) action = null - }, - retryOnNfcFailure = false + } ) private suspend fun useSessionUsb( diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/oath/OathManager.kt b/android/app/src/main/kotlin/com/yubico/authenticator/oath/OathManager.kt index 16952fa6..70640f98 100644 --- a/android/app/src/main/kotlin/com/yubico/authenticator/oath/OathManager.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/oath/OathManager.kt @@ -110,6 +110,15 @@ class OathManager( private val updateDeviceInfo = AtomicBoolean(false) private var deviceInfoTimer: TimerTask? = null + override fun onError() { + super.onError() + logger.debug("Cancel any pending action because of upstream error") + pendingAction?.let { action -> + action.invoke(Result.failure(CancellationException())) + pendingAction = null + } + } + override fun onPause() { deviceInfoTimer?.cancel() // cancel any pending actions, except for addToAny @@ -217,7 +226,8 @@ class OathManager( coroutineScope.cancel() } - override suspend fun processYubiKey(device: YubiKeyDevice) { + override suspend fun processYubiKey(device: YubiKeyDevice): Boolean { + var requestHandled = true try { device.withConnection { connection -> val session = getOathSession(connection) @@ -227,6 +237,8 @@ class OathManager( if (pendingAction != null) { pendingAction?.let { action -> pendingAction = null + // it is the pending action who handles this request + requestHandled = false action.invoke(Result.success(session)) } } else { @@ -235,7 +247,7 @@ class OathManager( try { oathViewModel.updateCredentials(calculateOathCodes(session)) } catch (error: Exception) { - logger.error("Failed to refresh codes", error) + logger.error("Failed to refresh codes: ", error) throw error } } @@ -263,7 +275,9 @@ class OathManager( if (addToAny) { // Special "add to any YubiKey" action, process addToAny = false + requestHandled = false action.invoke(Result.success(session)) + requestHandled = true } else { // Awaiting an action for a different device? Fail it and stop processing. action.invoke(Result.failure(IllegalStateException("Wrong deviceId"))) @@ -305,8 +319,17 @@ class OathManager( logger.error("Failed to connect to CCID: ", e) // Clear any cached OATH state oathViewModel.clearSession() + // Remove any pending action + pendingAction?.let { action -> + logger.error("Cancelling pending action") + pendingAction = null + action.invoke(Result.failure(CancellationException())) + } + throw e } + + return requestHandled } private suspend fun addAccountToAny( @@ -316,7 +339,7 @@ class OathManager( val credentialData: CredentialData = CredentialData.parseUri(URI.create(uri)) addToAny = true - return useOathSession(retryOnNfcFailure = false) { session -> + return useOathSession { session -> // We need to check for duplicates here since we haven't yet read the credentials if (session.credentials.any { it.id.contentEquals(credentialData.id) }) { throw IllegalArgumentException() @@ -346,7 +369,7 @@ class OathManager( logger.trace("Adding following accounts: {}", uris) addToAny = true - return useOathSession(retryOnNfcFailure = false) { session -> + return useOathSession { session -> var successCount = 0 for (index in uris.indices) { @@ -398,7 +421,16 @@ class OathManager( val remembered = keyManager.isRemembered(it.deviceId) if (unlocked) { oathViewModel.setSessionState(Session(it, remembered)) - oathViewModel.updateCredentials(calculateOathCodes(it)) + + try { + oathViewModel.updateCredentials(calculateOathCodes(it)) + } catch (e: Exception) { + // after unlocking there was problem getting the codes + // to avoid incomplete session, just reset it so that the user has to + // unlock it again + oathViewModel.clearSession() + throw e + } } jsonSerializer.encodeToString(mapOf("unlocked" to unlocked, "remembered" to remembered)) @@ -682,7 +714,6 @@ class OathManager( private suspend fun useOathSession( unlock: Boolean = true, updateDeviceInfo: Boolean = false, - retryOnNfcFailure: Boolean = true, block: (YubiKitOathSession) -> T ): T { // callers can decide whether the session should be unlocked first @@ -695,8 +726,7 @@ class OathManager( onCancelled = { pendingAction?.invoke(Result.failure(CancellationException())) pendingAction = null - }, - retryOnNfcFailure = retryOnNfcFailure + } ) } @@ -722,7 +752,6 @@ class OathManager( block.invoke(it.value) }) } - // here the coroutine is suspended and waits till pendingAction is // invoked - the pending action result will resume this coroutine } diff --git a/lib/android/overlay/nfc/nfc_overlay.dart b/lib/android/overlay/nfc/nfc_overlay.dart index 53325fda..028a4d7b 100755 --- a/lib/android/overlay/nfc/nfc_overlay.dart +++ b/lib/android/overlay/nfc/nfc_overlay.dart @@ -28,7 +28,7 @@ import 'views/nfc_content_widget.dart'; import 'views/nfc_overlay_icons.dart'; import 'views/nfc_overlay_widget.dart'; -final _log = Logger('android.tap_request_dialog'); +final _log = Logger('android.nfc_overlay'); const _channel = MethodChannel('com.yubico.authenticator.channel.nfc_overlay'); final nfcOverlay = @@ -41,6 +41,7 @@ class _NfcOverlayNotifier extends Notifier { @override int build() { ref.listen(androidNfcState, (previous, current) { + _log.debug('Received nfc state: $current'); processingViewTimeout?.cancel(); final notifier = ref.read(nfcEventNotifier.notifier); @@ -62,6 +63,8 @@ class _NfcOverlayNotifier extends Notifier { break; case NfcState.failure: notifier.send(showFailed()); + notifier + .send(const NfcHideViewEvent(delay: Duration(milliseconds: 800))); break; case NfcState.disabled: _log.debug('Received state: disabled'); @@ -125,7 +128,7 @@ class _NfcOverlayNotifier extends Notifier { } NfcEvent showFailed() { - ref.read(nfcOverlayWidgetProperties.notifier).update(hasCloseButton: true); + ref.read(nfcOverlayWidgetProperties.notifier).update(hasCloseButton: false); return NfcSetViewEvent( child: NfcContentWidget( title: l10n.s_nfc_ready_to_scan, diff --git a/lib/app/views/main_page.dart b/lib/app/views/main_page.dart index ebf52c96..4d088453 100755 --- a/lib/app/views/main_page.dart +++ b/lib/app/views/main_page.dart @@ -59,33 +59,24 @@ class MainPage extends ConsumerWidget { // If the current device changes, we need to pop any open dialogs. ref.listen>(currentDeviceDataProvider, (prev, next) { - var canPop = true; - if ((next.value != null) && (prev?.value != null)) { - // if there is change only in fipsApproved, don't pop anything - var nextInfo = next.value!.info; - var prevInfo = prev!.value!.info; - - canPop = - prevInfo.copyWith(fipsApproved: nextInfo.fipsApproved) != nextInfo; - } else if (next.hasValue && (prev != null && prev.isLoading)) { - canPop = false; + final serial = next.value?.info.serial; + if (serial != null && serial == prev?.value?.info.serial) { + return; } - if (canPop) { - Navigator.of(context).popUntil((route) { - return route.isFirst || - [ - 'device_picker', - 'settings', - 'about', - 'licenses', - 'user_interaction_prompt', - 'oath_add_account', - 'oath_icon_pack_dialog', - 'android_qr_scanner_view', - ].contains(route.settings.name); - }); - } + Navigator.of(context).popUntil((route) { + return route.isFirst || + [ + 'device_picker', + 'settings', + 'about', + 'licenses', + 'user_interaction_prompt', + 'oath_add_account', + 'oath_icon_pack_dialog', + 'android_qr_scanner_view', + ].contains(route.settings.name); + }); }); final deviceNode = ref.watch(currentDeviceProvider);