Change wallet defaults and fix race condition.

Make the wallet app default to NFC Negotiated Handover and close the connection after sending the response. This change is to more closely mimic wallets already in production using this code. Also add additional checks for callback inhibition when inside the executor to avoid transport callbacks after closing a connection. This happened in the reader app - causing a crash - when the mDL included status 20 in the same SessionData message which contained the result (e.g. using auto-close). Test: Manually tested. Test: All unit tests pass. Signed-off-by: David Zeuthen <[email protected]>
openwallet-foundation-labs · Dec 11, 2023 · de7d101 · de7d101
1 parent e5a6a4f
commit de7d101
Show file tree

Hide file tree

Showing 5 changed files with 122 additions and 43 deletions.
diff --git a/appholder/src/main/java/com/android/identity/wallet/util/PreferencesHelper.kt b/appholder/src/main/java/com/android/identity/wallet/util/PreferencesHelper.kt
@@ -87,15 +87,15 @@ object PreferencesHelper {
     }
 
     fun isConnectionAutoCloseEnabled(): Boolean {
-        return sharedPreferences.getBoolean(CONNECTION_AUTO_CLOSE, false)
+        return sharedPreferences.getBoolean(CONNECTION_AUTO_CLOSE, true)
     }
 
     fun setConnectionAutoCloseEnabled(enabled: Boolean) {
         sharedPreferences.edit { putBoolean(CONNECTION_AUTO_CLOSE, enabled) }
     }
 
     fun shouldUseStaticHandover(): Boolean {
-        return sharedPreferences.getBoolean(STATIC_HANDOVER, true)
+        return sharedPreferences.getBoolean(STATIC_HANDOVER, false)
     }
 
     fun setUseStaticHandover(enabled: Boolean) {

diff --git a/...rc/main/java/com/android/identity/android/mdoc/deviceretrieval/DeviceRetrievalHelper.java b/...rc/main/java/com/android/identity/android/mdoc/deviceretrieval/DeviceRetrievalHelper.java
@@ -99,17 +99,25 @@ void reportEReaderKeyReceived(@NonNull PublicKey eReaderKey) {
         Logger.d(TAG, "reportEReaderKeyReceived: " + eReaderKey);
         final Listener listener = mListener;
         final Executor executor = mListenerExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(() -> listener.onEReaderKeyReceived(eReaderKey));
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onEReaderKeyReceived(eReaderKey);
+                }
+            });
         }
     }
 
     void reportDeviceRequest(@NonNull byte[] deviceRequestBytes) {
         Logger.d(TAG, "reportDeviceRequest: deviceRequestBytes: " + deviceRequestBytes.length + " bytes");
         final Listener listener = mListener;
         final Executor executor = mListenerExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(() -> listener.onDeviceRequest(deviceRequestBytes));
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onDeviceRequest(deviceRequestBytes);
+                }
+            });
         }
     }
 
@@ -118,18 +126,25 @@ void reportDeviceDisconnected(boolean transportSpecificTermination) {
                 + transportSpecificTermination);
         final Listener listener = mListener;
         final Executor executor = mListenerExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(() -> listener.onDeviceDisconnected(
-                    transportSpecificTermination));
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onDeviceDisconnected(transportSpecificTermination);
+                }
+            });
         }
     }
 
     void reportError(@NonNull Throwable error) {
         Logger.d(TAG, "reportError: error: ", error);
         final Listener listener = mListener;
         final Executor executor = mListenerExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(() -> listener.onError(error));
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onError(error);
+                }
+            });
         }
     }
 

diff --git a/...droid/src/main/java/com/android/identity/android/mdoc/engagement/NfcEngagementHelper.java b/...droid/src/main/java/com/android/identity/android/mdoc/engagement/NfcEngagementHelper.java
@@ -799,8 +799,12 @@ void reportTwoWayEngagementDetected() {
         Logger.d(TAG, "reportTwoWayEngagementDetected");
         final Listener listener = mListener;
         final Executor executor = mExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(listener::onTwoWayEngagementDetected);
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onTwoWayEngagementDetected();
+                }
+            });
         }
     }
 
@@ -809,26 +813,38 @@ void reportDeviceConnecting() {
         Logger.d(TAG, "reportDeviceConnecting");
         final Listener listener = mListener;
         final Executor executor = mExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(listener::onDeviceConnecting);
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onDeviceConnecting();
+                }
+            });
         }
     }
 
     void reportDeviceConnected(DataTransport transport) {
         Logger.d(TAG, "reportDeviceConnected");
         final Listener listener = mListener;
         final Executor executor = mExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(() -> listener.onDeviceConnected(transport));
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onDeviceConnected(transport);
+                }
+            });
         }
     }
 
     void reportError(@NonNull Throwable error) {
         Logger.d(TAG, "reportError: error: ", error);
         final Listener listener = mListener;
         final Executor executor = mExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(() -> listener.onError(error));
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onError(error);
+                }
+            });
         }
     }
 

diff --git a/...ndroid/src/main/java/com/android/identity/android/mdoc/engagement/QrEngagementHelper.java b/...ndroid/src/main/java/com/android/identity/android/mdoc/engagement/QrEngagementHelper.java
@@ -292,35 +292,51 @@ void reportDeviceEngagementReady() {
         Logger.d(TAG, "reportDeviceEngagementReady");
         final Listener listener = mListener;
         final Executor executor = mExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(listener::onDeviceEngagementReady);
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onDeviceEngagementReady();
+                }
+            });
         }
     }
 
     void reportDeviceConnecting() {
         Logger.d(TAG, "reportDeviceConnecting");
         final Listener listener = mListener;
         final Executor executor = mExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(listener::onDeviceConnecting);
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onDeviceConnecting();
+                }
+            });
         }
     }
 
     void reportDeviceConnected(DataTransport transport) {
         Logger.d(TAG, "reportDeviceConnected");
         final Listener listener = mListener;
         final Executor executor = mExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(() -> listener.onDeviceConnected(transport));
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onDeviceConnected(transport);
+                }
+            });
         }
     }
 
     void reportError(@NonNull Throwable error) {
         Logger.d(TAG, "reportError: error: ", error);
         final Listener listener = mListener;
         final Executor executor = mExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(() -> listener.onError(error));
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onError(error);
+                }
+            });
         }
     }
 

diff --git a/...tity-android/src/main/java/com/android/identity/android/mdoc/transport/DataTransport.java b/...tity-android/src/main/java/com/android/identity/android/mdoc/transport/DataTransport.java
@@ -211,65 +211,97 @@ protected void inhibitCallbacks() {
     protected void reportConnectionMethodReady() {
         final Listener listener = mListener;
         final Executor executor = mListenerExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(() -> listener.onConnectionMethodReady());
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onConnectionMethodReady();
+                }
+            });
         }
     }
 
     protected void reportConnecting() {
         final Listener listener = mListener;
         final Executor executor = mListenerExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(listener::onConnecting);
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onConnecting();
+                }
+            });
         }
     }
 
     protected void reportConnected() {
         final Listener listener = mListener;
         final Executor executor = mListenerExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(listener::onConnected);
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onConnected();
+                }
+            });
         }
     }
 
     protected void reportDisconnected() {
         final Listener listener = mListener;
         final Executor executor = mListenerExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(listener::onDisconnected);
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onDisconnected();
+                }
+            });
         }
     }
 
     protected void reportMessageReceived(@NonNull byte[] data) {
         mMessageReceivedQueue.add(data);
         final Listener listener = mListener;
         final Executor executor = mListenerExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(() -> listener.onMessageReceived());
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onMessageReceived();
+                }
+            });
         }
     }
 
     protected void reportMessageProgress(long progress, long max) {
         final TransmissionProgressListener listener = mProgressListener;
         final Executor executor = mProgressListenerExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(() -> listener.onProgressUpdate(progress, max));
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onProgressUpdate(progress, max);
+                }
+            });
         }
     }
 
     protected void reportTransportSpecificSessionTermination() {
         final Listener listener = mListener;
         final Executor executor = mListenerExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(listener::onTransportSpecificSessionTermination);
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onTransportSpecificSessionTermination();
+                }
+            });
         }
     }
 
     protected void reportError(@NonNull Throwable error) {
         final Listener listener = mListener;
         final Executor executor = mListenerExecutor;
-        if (!mInhibitCallbacks && listener != null && executor != null) {
-            executor.execute(() -> listener.onError(error));
+        if (listener != null && executor != null) {
+            executor.execute(() -> {
+                if (!mInhibitCallbacks) {
+                    listener.onError(error);
+                }
+            });
         }
     }