Merge "Check location permission for ConnDiags last."

framework/src/android/net/ConnectivityDiagnosticsManager.java

@@ -713,7 +713,9 @@ public class ConnectivityDiagnosticsManager {
      * <p>Callbacks registered by apps not meeting the above criteria will not be invoked.
      *
      * <p>If a registering app loses its relevant permissions, any callbacks it registered will
-     * silently stop receiving callbacks.
+     * silently stop receiving callbacks. Note that registering apps must also have location
+     * permissions to receive callbacks as some Networks may be location-bound (such as WiFi
+     * networks).
      *
      * <p>Each register() call <b>MUST</b> use a ConnectivityDiagnosticsCallback instance that is
      * not currently registered. If a ConnectivityDiagnosticsCallback instance is registered with

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

@@ -9171,6 +9171,34 @@ public class ConnectivityService extends IConnectivityManager.Stub
         return results;
     }
 
+    private boolean hasLocationPermission(String packageName, int uid) {
+        // LocationPermissionChecker#checkLocationPermission can throw SecurityException if the uid
+        // and package name don't match. Throwing on the CS thread is not acceptable, so wrap the
+        // call in a try-catch.
+        try {
+            if (!mLocationPermissionChecker.checkLocationPermission(
+                        packageName, null /* featureId */, uid, null /* message */)) {
+                return false;
+            }
+        } catch (SecurityException e) {
+            return false;
+        }
+
+        return true;
+    }
+
+    private boolean ownsVpnRunningOverNetwork(int uid, Network network) {
+        for (NetworkAgentInfo virtual : mNetworkAgentInfos) {
+            if (virtual.supportsUnderlyingNetworks()
+                    && virtual.networkCapabilities.getOwnerUid() == uid
+                    && CollectionUtils.contains(virtual.declaredUnderlyingNetworks, network)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     @VisibleForTesting
     boolean checkConnectivityDiagnosticsPermissions(
             int callbackPid, int callbackUid, NetworkAgentInfo nai, String callbackPackageName) {
@@ -9178,29 +9206,14 @@ public class ConnectivityService extends IConnectivityManager.Stub
             return true;
         }
 
-        // LocationPermissionChecker#checkLocationPermission can throw SecurityException if the uid
+        // Administrator UIDs also contains the Owner UID
-        // and package name don't match. Throwing on the CS thread is not acceptable, so wrap the
+        final int[] administratorUids = nai.networkCapabilities.getAdministratorUids();
-        // call in a try-catch.
+        if (!CollectionUtils.contains(administratorUids, callbackUid)
-        try {
+                && !ownsVpnRunningOverNetwork(callbackUid, nai.network)) {
-            if (!mLocationPermissionChecker.checkLocationPermission(
-                    callbackPackageName, null /* featureId */, callbackUid, null /* message */)) {
-                return false;
-            }
-        } catch (SecurityException e) {
             return false;
         }
 
-        for (NetworkAgentInfo virtual : mNetworkAgentInfos) {
+        return hasLocationPermission(callbackPackageName, callbackUid);
-            if (virtual.supportsUnderlyingNetworks()
-                    && virtual.networkCapabilities.getOwnerUid() == callbackUid
-                    && CollectionUtils.contains(virtual.declaredUnderlyingNetworks, nai.network)) {
-                return true;
-            }
-        }
-
-        // Administrator UIDs also contains the Owner UID
-        final int[] administratorUids = nai.networkCapabilities.getAdministratorUids();
-        return CollectionUtils.contains(administratorUids, callbackUid);
     }
 
     @Override

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

@@ -9743,28 +9743,32 @@ public class ConnectivityServiceTest {
 
     @Test
     public void testCheckConnectivityDiagnosticsPermissionsWrongUidPackageName() throws Exception {
-        final NetworkAgentInfo naiWithoutUid = fakeMobileNai(new NetworkCapabilities());
+        final int wrongUid = Process.myUid() + 1;
+
+        final NetworkCapabilities nc = new NetworkCapabilities();
+        nc.setAdministratorUids(new int[] {wrongUid});
+        final NetworkAgentInfo naiWithUid = fakeMobileNai(nc);
 
         mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED);
 
         assertFalse(
                 "Mismatched uid/package name should not pass the location permission check",
                 mService.checkConnectivityDiagnosticsPermissions(
-                        Process.myPid() + 1, Process.myUid() + 1, naiWithoutUid,
+                        Process.myPid() + 1, wrongUid, naiWithUid, mContext.getOpPackageName()));
-                        mContext.getOpPackageName()));
     }
 
     @Test
     public void testCheckConnectivityDiagnosticsPermissionsNoLocationPermission() throws Exception {
-        final NetworkAgentInfo naiWithoutUid = fakeMobileNai(new NetworkCapabilities());
+        final NetworkCapabilities nc = new NetworkCapabilities();
+        nc.setAdministratorUids(new int[] {Process.myUid()});
+        final NetworkAgentInfo naiWithUid = fakeMobileNai(nc);
 
         mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED);
 
         assertFalse(
                 "ACCESS_FINE_LOCATION permission necessary for Connectivity Diagnostics",
                 mService.checkConnectivityDiagnosticsPermissions(
-                        Process.myPid(), Process.myUid(), naiWithoutUid,
+                        Process.myPid(), Process.myUid(), naiWithUid, mContext.getOpPackageName()));
-                        mContext.getOpPackageName()));
     }
 
     @Test