Merge PR Yubico#1688

android/app/src/main/kotlin/com/yubico/authenticator/AppContextManager.kt

@@ -28,7 +28,7 @@ abstract class AppContextManager {
 
     open fun onPause() {}
 
-    open fun onError() {}
+    open fun onError(e: Exception) {}
 }
 
 class ContextDisposedException : Exception()

android/app/src/main/kotlin/com/yubico/authenticator/MainActivity.kt

@@ -79,6 +79,7 @@ import kotlinx.coroutines.launch
 import org.json.JSONObject
 import org.slf4j.LoggerFactory
 import java.io.Closeable
+import java.io.IOException
 import java.security.NoSuchAlgorithmException
 import java.util.concurrent.Executors
 import javax.crypto.Mac
@@ -310,43 +311,55 @@ class MainActivity : FlutterFragmentActivity() {
     }
 
     private suspend fun processYubiKey(device: YubiKeyDevice) {
-        val deviceInfo = getDeviceInfo(device)
+        val deviceInfo = try {
 
-        if (deviceInfo == null) {
-            deviceManager.setDeviceInfo(null)
-            return
-        }
-
-        if (device is NfcYubiKeyDevice) {
-            appMethodChannel.nfcStateChanged(NfcState.ONGOING)
-        }
+            if (device is NfcYubiKeyDevice) {
+                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...")
-            deviceManager.scpKeyParams = try {
-                device.withConnection<SmartCardConnection, ScpKeyParams?> { connection ->
-                    val scp = SecurityDomainSession(connection)
-                    val keyRef = scp.keyInformation.keys.firstOrNull { it.kid == ScpKid.SCP11b }
-                    keyRef?.let {
-                        val certs = scp.getCertificateBundle(it)
-                        if (certs.isNotEmpty()) Scp11KeyParams(
-                            keyRef,
-                            certs[certs.size - 1].publicKey
-                        ) else null
-                    }?.also {
-                        logger.debug("Found SCP11b key: {}", keyRef)
+            val deviceInfo = getDeviceInfo(device)
+
+            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...")
+                deviceManager.scpKeyParams = try {
+                    device.withConnection<SmartCardConnection, ScpKeyParams?> { connection ->
+                        val scp = SecurityDomainSession(connection)
+                        val keyRef = scp.keyInformation.keys.firstOrNull { it.kid == ScpKid.SCP11b }
+                        keyRef?.let {
+                            val certs = scp.getCertificateBundle(it)
+                            if (certs.isNotEmpty()) Scp11KeyParams(
+                                keyRef,
+                                certs[certs.size - 1].publicKey
+                            ) else null
+                        }?.also {
+                            logger.debug("Found SCP11b key: {}", keyRef)
+                        }
                     }
+                } catch (e: Exception) {
+                    logger.error("Exception when reading SCP key information: ", e)
+                    // we throw IO exception to unify handling failures as we don't want
+                    // th clear device info
+                    throw IOException("Failure getting SCP keys")
                 }
-            } catch (e: Exception) {
-                logger.debug("Exception while getting scp keys: ", e)
-                contextManager?.onError()
-                if (device is NfcYubiKeyDevice) {
-                    appMethodChannel.nfcStateChanged(NfcState.FAILURE)
-                }
-                null
             }
+            deviceInfo
+        } catch (e: Exception) {
+            logger.debug("Exception while getting device info and scp keys: ", e)
+            contextManager?.onError(e)
+            if (device is NfcYubiKeyDevice) {
+                appMethodChannel.nfcStateChanged(NfcState.FAILURE)
+            }
+
+            // do not clear deviceInfo on IOExceptions,
+            // this allows for retries of failed actions
+            if (e !is IOException) {
+                logger.debug("Resetting device info")
+                deviceManager.setDeviceInfo(null)
+            }
+
+            return
         }
 
         // this YubiKey provides SCP11b key but the phone cannot perform AESCMAC

android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoConnectionHelper.kt

@@ -42,14 +42,6 @@ class FidoConnectionHelper(private val deviceManager: DeviceManager) {
         return requestHandled
     }
 
-    fun failPending(e: Exception) {
-        pendingAction?.let { action ->
-            logger.error("Failing pending action with {}", e.message)
-            action.invoke(Result.failure(e))
-            pendingAction = null
-        }
-    }
-
     fun cancelPending() {
         pendingAction?.let { action ->
             action.invoke(Result.failure(CancellationException()))
@@ -80,7 +72,7 @@ class FidoConnectionHelper(private val deviceManager: DeviceManager) {
         block(YubiKitFidoSession(it))
     }.also {
         if (updateDeviceInfo) {
-            deviceManager.setDeviceInfo(getDeviceInfo(device))
+            deviceManager.setDeviceInfo(runCatching { getDeviceInfo(device) }.getOrNull())
         }
     }
 

android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoManager.kt

@@ -174,9 +174,9 @@ class FidoManager(
         }
     }
 
-    override fun onError() {
-        super.onError()
-        logger.debug("Cancel any pending action because of upstream error")
+    override fun onError(e: Exception) {
+        super.onError(e)
+        logger.error("Cancelling pending action. Cause: ", e)
         connectionHelper.cancelPending()
     }
 
@@ -204,13 +204,12 @@ class FidoManager(
             }
 
             if (updateDeviceInfo.getAndSet(false)) {
-                deviceManager.setDeviceInfo(getDeviceInfo(device))
+                deviceManager.setDeviceInfo(runCatching { getDeviceInfo(device) }.getOrNull())
             }
         } catch (e: Exception) {
-            // something went wrong, try to get DeviceInfo from any available connection type
-            logger.error("Failure when processing YubiKey: ", e)
 
-            connectionHelper.failPending(e)
+            logger.error("Cancelling pending action. Cause: ", e)
+            connectionHelper.cancelPending()
 
             if (e !is IOException) {
                 // we don't clear the session on IOExceptions so that the session is ready for
@@ -240,7 +239,7 @@ class FidoManager(
             currentSession
         )
 
-        val sameDevice = currentSession == previousSession
+        val sameDevice = currentSession.sameDevice(previousSession)
 
         if (device is NfcYubiKeyDevice && (sameDevice || resetHelper.inProgress)) {
             requestHandled = connectionHelper.invokePending(fidoSession)

android/app/src/main/kotlin/com/yubico/authenticator/fido/data/Session.kt

@@ -41,6 +41,19 @@ data class Options(
         infoData.getOptionsBoolean("ep"),
     )
 
+    fun sameDevice(other: Options) : Boolean {
+        if (this === other) return true
+
+        if (clientPin != other.clientPin) return false
+        if (credMgmt != other.credMgmt) return false
+        if (credentialMgmtPreview != other.credentialMgmtPreview) return false
+        if (bioEnroll != other.bioEnroll) return false
+        // alwaysUv may differ
+        // ep may differ
+
+        return true
+    }
+
     companion object {
         private fun InfoData.getOptionsBoolean(
             key: String
@@ -67,6 +80,21 @@ data class SessionInfo(
         infoData.remainingDiscoverableCredentials
     )
 
+    // this is a more permissive comparison, which does not take in an account properties,
+    // which might change by using the FIDO authenticator
+    fun sameDevice(other: SessionInfo?): Boolean {
+        if (other == null) return false
+        if (this === other) return true
+
+        if (!options.sameDevice(other.options)) return false
+        if (!aaguid.contentEquals(other.aaguid)) return false
+        // minPinLength may differ
+        // forcePinChange may differ
+        // remainingDiscoverableCredentials may differ
+
+        return true
+    }
+
     override fun equals(other: Any?): Boolean {
         if (this === other) return true
         if (javaClass != other?.javaClass) return false

android/app/src/main/kotlin/com/yubico/authenticator/oath/OathManager.kt

@@ -110,9 +110,9 @@ 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")
+    override fun onError(e: Exception) {
+        super.onError(e)
+        logger.error("Cancelling pending action in onError. Cause: ", e)
         pendingAction?.let { action ->
             action.invoke(Result.failure(CancellationException()))
             pendingAction = null
@@ -343,16 +343,16 @@ class OathManager(
             )
 
             if (updateDeviceInfo.getAndSet(false)) {
-                deviceManager.setDeviceInfo(getDeviceInfo(device))
+                deviceManager.setDeviceInfo(runCatching { getDeviceInfo(device) }.getOrNull())
             }
         } catch (e: Exception) {
             // OATH not enabled/supported, try to get DeviceInfo over other USB interfaces
             logger.error("Exception during SmartCard connection/OATH session creation: ", e)
 
-            // Remove any pending action
+            // Cancel any pending action
             pendingAction?.let { action ->
-                logger.error("Failing pending action with {}", e.message)
-                action.invoke(Result.failure(e))
+                logger.error("Cancelling pending action. Cause: ", e)
+                action.invoke(Result.failure(CancellationException()))
                 pendingAction = null
             }
 
@@ -782,7 +782,7 @@ class OathManager(
         block(getOathSession(it))
     }.also {
         if (updateDeviceInfo) {
-            deviceManager.setDeviceInfo(getDeviceInfo(device))
+            deviceManager.setDeviceInfo(runCatching { getDeviceInfo(device) }.getOrNull())
         }
     }
 

android/app/src/main/kotlin/com/yubico/authenticator/yubikit/DeviceInfoHelper.kt

@@ -47,17 +47,17 @@ class DeviceInfoHelper {
         private val restrictedNfcBytes =
             byteArrayOf(0x00, 0x1F, 0xD1.toByte(), 0x01, 0x1b, 0x55, 0x04) + uri
 
-        suspend fun getDeviceInfo(device: YubiKeyDevice): Info? {
+        suspend fun getDeviceInfo(device: YubiKeyDevice): Info {
             SessionVersionOverride.set(null)
             var deviceInfo = readDeviceInfo(device)
-            if (deviceInfo?.version?.major == 0.toByte()) {
+            if (deviceInfo.version.major == 0.toByte()) {
                 SessionVersionOverride.set(Version(5, 7, 2))
                 deviceInfo = readDeviceInfo(device)
             }
             return deviceInfo
         }
 
-        private suspend fun readDeviceInfo(device: YubiKeyDevice): Info? {
+        private suspend fun readDeviceInfo(device: YubiKeyDevice): Info {
             val pid = (device as? UsbYubiKeyDevice)?.pid
 
             val deviceInfo = runCatching {
@@ -106,8 +106,8 @@ class DeviceInfoHelper {
                         }
                 } catch (e: Exception) {
                     // no smart card connectivity
-                    logger.error("Failure getting device info", e)
-                    return null
+                    logger.error("Failure getting device info: ", e)
+                    throw e
                 }
             }
 

lib/android/fido/state.dart

@@ -365,9 +365,8 @@ class _FidoCredentialsNotifier extends FidoCredentialsNotifier {
       var decodedException = pe.decode();
       if (decodedException is CancellationException) {
         _log.debug('User cancelled delete credential FIDO operation');
-      } else {
-        throw decodedException;
       }
+      throw decodedException;
     }
   }
 }

lib/app/views/main_page.dart

@@ -60,7 +60,8 @@ class MainPage extends ConsumerWidget {
       final prevSerial =
           prev?.hasValue == true ? prev?.value?.info.serial : null;
       if ((serial != null && serial == prevSerial) ||
-          (next.hasValue && (prev != null && prev.isLoading))) {
+          (next.hasValue && (prev != null && prev.isLoading)) ||
+          next.isLoading) {
         return;
       }
 

lib/fido/views/delete_credential_dialog.dart

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2022 Yubico.
+ * Copyright (C) 2022-2024 Yubico.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -23,6 +23,7 @@ import 'package:flutter_riverpod/flutter_riverpod.dart';
 import '../../app/message.dart';
 import '../../app/models.dart';
 import '../../app/state.dart';
+import '../../exception/cancellation_exception.dart';
 import '../../widgets/responsive_dialog.dart';
 import '../models.dart';
 import '../state.dart';
@@ -57,15 +58,19 @@ class DeleteCredentialDialog extends ConsumerWidget {
       actions: [
         TextButton(
           onPressed: () async {
-            await ref
-                .read(credentialProvider(devicePath).notifier)
-                .deleteCredential(credential);
-            await ref.read(withContextProvider)(
-              (context) async {
-                Navigator.of(context).pop(true);
-                showMessage(context, l10n.s_passkey_deleted);
-              },
-            );
+            try {
+              await ref
+                  .read(credentialProvider(devicePath).notifier)
+                  .deleteCredential(credential);
+              await ref.read(withContextProvider)(
+                (context) async {
+                  Navigator.of(context).pop(true);
+                  showMessage(context, l10n.s_passkey_deleted);
+                },
+              );
+            } on CancellationException catch (_) {
+              // ignored
+            }
           },
           child: Text(l10n.s_delete),
         ),