Report result SKIPPED in ConnDiags if the network is not validated. am: f1120be78b

Original change: https://android-review.googlesource.com/c/platform/packages/modules/Connectivity/+/1718510

Change-Id: I4135072382e447206ef7634e6e43b7ad5186a00d

service/src/com/android/server/ConnectivityService.java

@@ -55,6 +55,7 @@ import static android.net.ConnectivityManager.isNetworkTypeValid;
 import static android.net.ConnectivitySettingsManager.PRIVATE_DNS_MODE_OPPORTUNISTIC;
 import static android.net.INetworkMonitor.NETWORK_VALIDATION_PROBE_PRIVDNS;
 import static android.net.INetworkMonitor.NETWORK_VALIDATION_RESULT_PARTIAL;
+import static android.net.INetworkMonitor.NETWORK_VALIDATION_RESULT_SKIPPED;
 import static android.net.INetworkMonitor.NETWORK_VALIDATION_RESULT_VALID;
 import static android.net.NetworkCapabilities.NET_CAPABILITY_CAPTIVE_PORTAL;
 import static android.net.NetworkCapabilities.NET_CAPABILITY_ENTERPRISE;
@@ -541,9 +542,9 @@ public class ConnectivityService extends IConnectivityManager.Stub
     private static final int EVENT_SET_AVOID_UNVALIDATED = 35;
 
     /**
-     * used to trigger revalidation of a network.
+     * used to handle reported network connectivity. May trigger revalidation of a network.
      */
-    private static final int EVENT_REVALIDATE_NETWORK = 36;
+    private static final int EVENT_REPORT_NETWORK_CONNECTIVITY = 36;
 
     // Handle changes in Private DNS settings.
     private static final int EVENT_PRIVATE_DNS_SETTINGS_CHANGED = 37;
@@ -3541,8 +3542,14 @@ public class ConnectivityService extends IConnectivityManager.Stub
             final NetworkAgentInfo nai = getNetworkAgentInfoForNetId(mNetId);
             if (nai == null) return;
 
+            // NetworkMonitor reports the network validation result as a bitmask while
+            // ConnectivityDiagnostics treats this value as an int. Convert the result to a single
+            // logical value for ConnectivityDiagnostics.
+            final int validationResult = networkMonitorValidationResultToConnDiagsValidationResult(
+                    p.result);
+
             final PersistableBundle extras = new PersistableBundle();
-            extras.putInt(KEY_NETWORK_VALIDATION_RESULT, p.result);
+            extras.putInt(KEY_NETWORK_VALIDATION_RESULT, validationResult);
             extras.putInt(KEY_NETWORK_PROBES_SUCCEEDED_BITMASK, p.probesSucceeded);
             extras.putInt(KEY_NETWORK_PROBES_ATTEMPTED_BITMASK, p.probesAttempted);
 
@@ -3618,6 +3625,22 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
     }
 
+    /**
+     * Converts the given NetworkMonitor-specific validation result bitmask to a
+     * ConnectivityDiagnostics-specific validation result int.
+     */
+    private int networkMonitorValidationResultToConnDiagsValidationResult(int validationResult) {
+        if ((validationResult & NETWORK_VALIDATION_RESULT_SKIPPED) != 0) {
+            return ConnectivityReport.NETWORK_VALIDATION_RESULT_SKIPPED;
+        }
+        if ((validationResult & NETWORK_VALIDATION_RESULT_VALID) == 0) {
+            return ConnectivityReport.NETWORK_VALIDATION_RESULT_INVALID;
+        }
+        return (validationResult & NETWORK_VALIDATION_RESULT_PARTIAL) != 0
+                ? ConnectivityReport.NETWORK_VALIDATION_RESULT_PARTIALLY_VALID
+                : ConnectivityReport.NETWORK_VALIDATION_RESULT_VALID;
+    }
+
     private void notifyDataStallSuspected(DataStallReportParcelable p, int netId) {
         log("Data stall detected with methods: " + p.detectionMethod);
 
@@ -4867,8 +4890,9 @@ public class ConnectivityService extends IConnectivityManager.Stub
                     mKeepaliveTracker.handleStopKeepalive(nai, slot, reason);
                     break;
                 }
-                case EVENT_REVALIDATE_NETWORK: {
+                case EVENT_REPORT_NETWORK_CONNECTIVITY: {
-                    handleReportNetworkConnectivity((Network) msg.obj, msg.arg1, toBool(msg.arg2));
+                    handleReportNetworkConnectivity((NetworkAgentInfo) msg.obj, msg.arg1,
+                            toBool(msg.arg2));
                     break;
                 }
                 case EVENT_PRIVATE_DNS_SETTINGS_CHANGED:
@@ -5031,41 +5055,34 @@ public class ConnectivityService extends IConnectivityManager.Stub
         final int uid = mDeps.getCallingUid();
         final int connectivityInfo = encodeBool(hasConnectivity);
 
-        // Handle ConnectivityDiagnostics event before attempting to revalidate the network. This
-        // forces an ordering of ConnectivityDiagnostics events in the case where hasConnectivity
-        // does not match the known connectivity of the network - this causes NetworkMonitor to
-        // revalidate the network and generate a ConnectivityDiagnostics ConnectivityReport event.
         final NetworkAgentInfo nai;
         if (network == null) {
             nai = getDefaultNetwork();
         } else {
             nai = getNetworkAgentInfoForNetwork(network);
         }
-        if (nai != null) {
-            mConnectivityDiagnosticsHandler.sendMessage(
-                    mConnectivityDiagnosticsHandler.obtainMessage(
-                            ConnectivityDiagnosticsHandler.EVENT_NETWORK_CONNECTIVITY_REPORTED,
-                            connectivityInfo, 0, nai));
-        }
 
         mHandler.sendMessage(
-                mHandler.obtainMessage(EVENT_REVALIDATE_NETWORK, uid, connectivityInfo, network));
+                mHandler.obtainMessage(
+                        EVENT_REPORT_NETWORK_CONNECTIVITY, uid, connectivityInfo, nai));
     }
 
     private void handleReportNetworkConnectivity(
-            Network network, int uid, boolean hasConnectivity) {
+            @Nullable NetworkAgentInfo nai, int uid, boolean hasConnectivity) {
-        final NetworkAgentInfo nai;
+        // TODO(b/192611346): remove NetworkInfo.State.DISCONNECTING as it's not used
-        if (network == null) {
+        if (nai == null
-            nai = getDefaultNetwork();
+                || nai != getNetworkAgentInfoForNetwork(nai.network)
-        } else {
+                || nai.networkInfo.getState() == NetworkInfo.State.DISCONNECTING
-            nai = getNetworkAgentInfoForNetwork(network);
+                || nai.networkInfo.getState() == NetworkInfo.State.DISCONNECTED) {
-        }
-        if (nai == null || nai.networkInfo.getState() == NetworkInfo.State.DISCONNECTING ||
-            nai.networkInfo.getState() == NetworkInfo.State.DISCONNECTED) {
             return;
         }
         // Revalidate if the app report does not match our current validated state.
         if (hasConnectivity == nai.lastValidated) {
+            mConnectivityDiagnosticsHandler.sendMessage(
+                    mConnectivityDiagnosticsHandler.obtainMessage(
+                            ConnectivityDiagnosticsHandler.EVENT_NETWORK_CONNECTIVITY_REPORTED,
+                            new ReportedNetworkConnectivityInfo(
+                                    hasConnectivity, false /* isNetworkRevalidating */, uid, nai)));
             return;
         }
         if (DBG) {
@@ -5081,6 +5098,16 @@ public class ConnectivityService extends IConnectivityManager.Stub
         if (isNetworkWithCapabilitiesBlocked(nc, uid, false)) {
             return;
         }
+
+        // Send CONNECTIVITY_REPORTED event before re-validating the Network to force an ordering of
+        // ConnDiags events. This ensures that #onNetworkConnectivityReported() will be called
+        // before #onConnectivityReportAvailable(), which is called once Network evaluation is
+        // completed.
+        mConnectivityDiagnosticsHandler.sendMessage(
+                mConnectivityDiagnosticsHandler.obtainMessage(
+                        ConnectivityDiagnosticsHandler.EVENT_NETWORK_CONNECTIVITY_REPORTED,
+                        new ReportedNetworkConnectivityInfo(
+                                hasConnectivity, true /* isNetworkRevalidating */, uid, nai)));
         nai.networkMonitor().forceReevaluation(uid);
     }
 
@@ -9040,8 +9067,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
          * the platform. This event will invoke {@link
          * IConnectivityDiagnosticsCallback#onNetworkConnectivityReported} for permissioned
          * callbacks.
-         * obj = Network that was reported on
+         * obj = ReportedNetworkConnectivityInfo with info on reported Network connectivity.
-         * arg1 = boolint for the quality reported
          */
         private static final int EVENT_NETWORK_CONNECTIVITY_REPORTED = 5;
 
@@ -9079,7 +9105,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
                     break;
                 }
                 case EVENT_NETWORK_CONNECTIVITY_REPORTED: {
-                    handleNetworkConnectivityReported((NetworkAgentInfo) msg.obj, toBool(msg.arg1));
+                    handleNetworkConnectivityReported((ReportedNetworkConnectivityInfo) msg.obj);
                     break;
                 }
                 default: {
@@ -9149,6 +9175,28 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
     }
 
+    /**
+     * Class used for sending info for a call to {@link #reportNetworkConnectivity()} to {@link
+     * ConnectivityDiagnosticsHandler}.
+     */
+    private static class ReportedNetworkConnectivityInfo {
+        public final boolean hasConnectivity;
+        public final boolean isNetworkRevalidating;
+        public final int reporterUid;
+        @NonNull public final NetworkAgentInfo nai;
+
+        private ReportedNetworkConnectivityInfo(
+                boolean hasConnectivity,
+                boolean isNetworkRevalidating,
+                int reporterUid,
+                @NonNull NetworkAgentInfo nai) {
+            this.hasConnectivity = hasConnectivity;
+            this.isNetworkRevalidating = isNetworkRevalidating;
+            this.reporterUid = reporterUid;
+            this.nai = nai;
+        }
+    }
+
     private void handleRegisterConnectivityDiagnosticsCallback(
             @NonNull ConnectivityDiagnosticsCallbackInfo cbInfo) {
         ensureRunningOnConnectivityServiceThread();
@@ -9256,13 +9304,14 @@ public class ConnectivityService extends IConnectivityManager.Stub
                         networkCapabilities,
                         extras);
         nai.setConnectivityReport(report);
+
         final List<IConnectivityDiagnosticsCallback> results =
-                getMatchingPermissionedCallbacks(nai);
+                getMatchingPermissionedCallbacks(nai, Process.INVALID_UID);
         for (final IConnectivityDiagnosticsCallback cb : results) {
             try {
                 cb.onConnectivityReportAvailable(report);
             } catch (RemoteException ex) {
-                loge("Error invoking onConnectivityReport", ex);
+                loge("Error invoking onConnectivityReportAvailable", ex);
             }
         }
     }
@@ -9281,7 +9330,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
                         networkCapabilities,
                         extras);
         final List<IConnectivityDiagnosticsCallback> results =
-                getMatchingPermissionedCallbacks(nai);
+                getMatchingPermissionedCallbacks(nai, Process.INVALID_UID);
         for (final IConnectivityDiagnosticsCallback cb : results) {
             try {
                 cb.onDataStallSuspected(report);
@@ -9292,15 +9341,39 @@ public class ConnectivityService extends IConnectivityManager.Stub
     }
 
     private void handleNetworkConnectivityReported(
-            @NonNull NetworkAgentInfo nai, boolean connectivity) {
+            @NonNull ReportedNetworkConnectivityInfo reportedNetworkConnectivityInfo) {
+        final NetworkAgentInfo nai = reportedNetworkConnectivityInfo.nai;
+        final ConnectivityReport cachedReport = nai.getConnectivityReport();
+
+        // If the Network is being re-validated as a result of this call to
+        // reportNetworkConnectivity(), notify all permissioned callbacks. Otherwise, only notify
+        // permissioned callbacks registered by the reporter.
         final List<IConnectivityDiagnosticsCallback> results =
-                getMatchingPermissionedCallbacks(nai);
+                getMatchingPermissionedCallbacks(
+                        nai,
+                        reportedNetworkConnectivityInfo.isNetworkRevalidating
+                                ? Process.INVALID_UID
+                                : reportedNetworkConnectivityInfo.reporterUid);
+
         for (final IConnectivityDiagnosticsCallback cb : results) {
             try {
-                cb.onNetworkConnectivityReported(nai.network, connectivity);
+                cb.onNetworkConnectivityReported(
+                        nai.network, reportedNetworkConnectivityInfo.hasConnectivity);
             } catch (RemoteException ex) {
                 loge("Error invoking onNetworkConnectivityReported", ex);
             }
+
+            // If the Network isn't re-validating, also provide the cached report. If there is no
+            // cached report, the Network is still being validated and a report will be sent once
+            // validation is complete. Note that networks which never undergo validation will still
+            // have a cached ConnectivityReport with RESULT_SKIPPED.
+            if (!reportedNetworkConnectivityInfo.isNetworkRevalidating && cachedReport != null) {
+                try {
+                    cb.onConnectivityReportAvailable(cachedReport);
+                } catch (RemoteException ex) {
+                    loge("Error invoking onConnectivityReportAvailable", ex);
+                }
+            }
         }
     }
 
@@ -9313,20 +9386,38 @@ public class ConnectivityService extends IConnectivityManager.Stub
         return sanitized;
     }
 
+    /**
+     * Gets a list of ConnectivityDiagnostics callbacks that match the specified Network and uid.
+     *
+     * <p>If Process.INVALID_UID is specified, all matching callbacks will be returned.
+     */
     private List<IConnectivityDiagnosticsCallback> getMatchingPermissionedCallbacks(
-            @NonNull NetworkAgentInfo nai) {
+            @NonNull NetworkAgentInfo nai, int uid) {
         final List<IConnectivityDiagnosticsCallback> results = new ArrayList<>();
         for (Entry<IBinder, ConnectivityDiagnosticsCallbackInfo> entry :
                 mConnectivityDiagnosticsCallbacks.entrySet()) {
             final ConnectivityDiagnosticsCallbackInfo cbInfo = entry.getValue();
             final NetworkRequestInfo nri = cbInfo.mRequestInfo;
+
             // Connectivity Diagnostics rejects multilayer requests at registration hence get(0).
-            if (nai.satisfies(nri.mRequests.get(0))) {
+            if (!nai.satisfies(nri.mRequests.get(0))) {
-                if (checkConnectivityDiagnosticsPermissions(
+                continue;
-                        nri.mPid, nri.mUid, nai, cbInfo.mCallingPackageName)) {
-                    results.add(entry.getValue().mCb);
-                }
             }
+
+            // UID for this callback must either be:
+            //  - INVALID_UID (which sends callbacks to all UIDs), or
+            //  - The callback's owner (the owner called reportNetworkConnectivity() and is being
+            //    notified as a result)
+            if (uid != Process.INVALID_UID && uid != nri.mUid) {
+                continue;
+            }
+
+            if (!checkConnectivityDiagnosticsPermissions(
+                    nri.mPid, nri.mUid, nai, cbInfo.mCallingPackageName)) {
+                continue;
+            }
+
+            results.add(entry.getValue().mCb);
         }
         return results;
     }

tests/unit/java/com/android/server/ConnectivityServiceTest.java

@@ -10221,6 +10221,9 @@ public class ConnectivityServiceTest {
     public void testConnectivityDiagnosticsCallbackOnConnectivityReported() throws Exception {
         setUpConnectivityDiagnosticsCallback();
 
+        // reset to ignore callbacks from setup
+        reset(mConnectivityDiagnosticsCallback);
+
         final Network n = mCellNetworkAgent.getNetwork();
         final boolean hasConnectivity = true;
         mService.reportNetworkConnectivity(n, hasConnectivity);
@@ -10231,6 +10234,8 @@ public class ConnectivityServiceTest {
         // Verify onNetworkConnectivityReported fired
         verify(mConnectivityDiagnosticsCallback)
                 .onNetworkConnectivityReported(eq(n), eq(hasConnectivity));
+        verify(mConnectivityDiagnosticsCallback).onConnectivityReportAvailable(
+                argThat(report -> areConnDiagCapsRedacted(report.getNetworkCapabilities())));
 
         final boolean noConnectivity = false;
         mService.reportNetworkConnectivity(n, noConnectivity);
@@ -10241,6 +10246,54 @@ public class ConnectivityServiceTest {
         // Wait for onNetworkConnectivityReported to fire
         verify(mConnectivityDiagnosticsCallback)
                 .onNetworkConnectivityReported(eq(n), eq(noConnectivity));
+
+        // Also expect a ConnectivityReport after NetworkMonitor asynchronously re-validates
+        verify(mConnectivityDiagnosticsCallback, timeout(TIMEOUT_MS).times(2))
+                .onConnectivityReportAvailable(
+                        argThat(report ->
+                                areConnDiagCapsRedacted(report.getNetworkCapabilities())));
+    }
+
+    @Test
+    public void testConnectivityDiagnosticsCallbackOnConnectivityReportedSeparateUid()
+            throws Exception {
+        setUpConnectivityDiagnosticsCallback();
+
+        // reset to ignore callbacks from setup
+        reset(mConnectivityDiagnosticsCallback);
+
+        // report known Connectivity from a different uid. Verify that network is not re-validated
+        // and this callback is not notified.
+        final Network n = mCellNetworkAgent.getNetwork();
+        final boolean hasConnectivity = true;
+        doAsUid(Process.myUid() + 1, () -> mService.reportNetworkConnectivity(n, hasConnectivity));
+
+        // Block until all other events are done processing.
+        HandlerUtils.waitForIdle(mCsHandlerThread, TIMEOUT_MS);
+
+        // Verify onNetworkConnectivityReported did not fire
+        verify(mConnectivityDiagnosticsCallback, never())
+                .onNetworkConnectivityReported(any(), anyBoolean());
+        verify(mConnectivityDiagnosticsCallback, never())
+                .onConnectivityReportAvailable(any());
+
+        // report different Connectivity from a different uid. Verify that network is re-validated
+        // and that this callback is notified.
+        final boolean noConnectivity = false;
+        doAsUid(Process.myUid() + 1, () -> mService.reportNetworkConnectivity(n, noConnectivity));
+
+        // Block until all other events are done processing.
+        HandlerUtils.waitForIdle(mCsHandlerThread, TIMEOUT_MS);
+
+        // Wait for onNetworkConnectivityReported to fire
+        verify(mConnectivityDiagnosticsCallback, timeout(TIMEOUT_MS))
+                .onNetworkConnectivityReported(eq(n), eq(noConnectivity));
+
+        // Also expect a ConnectivityReport after NetworkMonitor asynchronously re-validates
+        verify(mConnectivityDiagnosticsCallback, timeout(TIMEOUT_MS))
+                .onConnectivityReportAvailable(
+                        argThat(report ->
+                                areConnDiagCapsRedacted(report.getNetworkCapabilities())));
     }
 
     @Test