Merge "Require location permission for ConnDiags WiFi only."

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

@@ -77,6 +77,7 @@ import static android.net.NetworkCapabilities.REDACT_FOR_NETWORK_SETTINGS;
 import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR;
 import static android.net.NetworkCapabilities.TRANSPORT_TEST;
 import static android.net.NetworkCapabilities.TRANSPORT_VPN;
+import static android.net.NetworkCapabilities.TRANSPORT_WIFI;
 import static android.net.NetworkRequest.Type.LISTEN_FOR_BEST;
 import static android.net.shared.NetworkMonitorUtils.isPrivateDnsValidationRequired;
 import static android.os.Process.INVALID_UID;
@@ -9186,6 +9187,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
         return results;
     }
 
+    private boolean isLocationPermissionRequiredForConnectivityDiagnostics(
+            @NonNull NetworkAgentInfo nai) {
+        // TODO(b/188483916): replace with a transport-agnostic location-aware check
+        return nai.networkCapabilities.hasTransport(TRANSPORT_WIFI);
+    }
+
     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
@@ -9228,7 +9235,8 @@ public class ConnectivityService extends IConnectivityManager.Stub
             return false;
         }
 
-        return hasLocationPermission(callbackPackageName, callbackUid);
+        return !isLocationPermissionRequiredForConnectivityDiagnostics(nai)
+                || hasLocationPermission(callbackPackageName, callbackUid);
     }
 
     @Override

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

@@ -9827,10 +9827,24 @@ public class ConnectivityServiceTest {
     }
 
     public NetworkAgentInfo fakeMobileNai(NetworkCapabilities nc) {
+        final NetworkCapabilities cellNc = new NetworkCapabilities.Builder(nc)
+                .addTransportType(TRANSPORT_CELLULAR).build();
         final NetworkInfo info = new NetworkInfo(TYPE_MOBILE, TelephonyManager.NETWORK_TYPE_LTE,
                 ConnectivityManager.getNetworkTypeName(TYPE_MOBILE),
                 TelephonyManager.getNetworkTypeName(TelephonyManager.NETWORK_TYPE_LTE));
-        return new NetworkAgentInfo(null, new Network(NET_ID), info, new LinkProperties(),
+        return fakeNai(cellNc, info);
+    }
+
+    private NetworkAgentInfo fakeWifiNai(NetworkCapabilities nc) {
+        final NetworkCapabilities wifiNc = new NetworkCapabilities.Builder(nc)
+                .addTransportType(TRANSPORT_WIFI).build();
+        final NetworkInfo info = new NetworkInfo(TYPE_WIFI, 0 /* subtype */,
+                ConnectivityManager.getNetworkTypeName(TYPE_WIFI), "" /* subtypeName */);
+        return fakeNai(wifiNc, info);
+    }
+
+    private NetworkAgentInfo fakeNai(NetworkCapabilities nc, NetworkInfo networkInfo) {
+        return new NetworkAgentInfo(null, new Network(NET_ID), networkInfo, new LinkProperties(),
                 nc, new NetworkScore.Builder().setLegacyInt(0).build(),
                 mServiceContext, null, new NetworkAgentConfig(), mService, null, null, 0,
                 INVALID_UID, mQosCallbackTracker, new ConnectivityService.Dependencies());
@@ -9855,7 +9869,7 @@ public class ConnectivityServiceTest {
 
         final NetworkCapabilities nc = new NetworkCapabilities();
         nc.setAdministratorUids(new int[] {wrongUid});
-        final NetworkAgentInfo naiWithUid = fakeMobileNai(nc);
+        final NetworkAgentInfo naiWithUid = fakeWifiNai(nc);
 
         mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED);
 
@@ -9865,18 +9879,37 @@ public class ConnectivityServiceTest {
                         Process.myPid() + 1, wrongUid, naiWithUid, mContext.getOpPackageName()));
     }
 
+    private void verifyConnectivityDiagnosticsPermissionsWithNetworkAgentInfo(
+            NetworkAgentInfo info, boolean expectPermission) {
+        mServiceContext.setPermission(android.Manifest.permission.NETWORK_STACK, PERMISSION_DENIED);
+
+        assertEquals(
+                "Unexpected ConnDiags permission",
+                expectPermission,
+                mService.checkConnectivityDiagnosticsPermissions(
+                        Process.myPid(), Process.myUid(), info, mContext.getOpPackageName()));
+    }
+
     @Test
-    public void testCheckConnectivityDiagnosticsPermissionsNoLocationPermission() throws Exception {
+    public void testCheckConnectivityDiagnosticsPermissionsCellularNoLocationPermission()
+            throws Exception {
         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);
+        verifyConnectivityDiagnosticsPermissionsWithNetworkAgentInfo(naiWithUid,
+                true /* expectPermission */);
+    }
 
-        assertFalse(
+    @Test
-                "ACCESS_FINE_LOCATION permission necessary for Connectivity Diagnostics",
+    public void testCheckConnectivityDiagnosticsPermissionsWifiNoLocationPermission()
-                mService.checkConnectivityDiagnosticsPermissions(
+            throws Exception {
-                        Process.myPid(), Process.myUid(), naiWithUid, mContext.getOpPackageName()));
+        final NetworkCapabilities nc = new NetworkCapabilities();
+        nc.setAdministratorUids(new int[] {Process.myUid()});
+        final NetworkAgentInfo naiWithUid = fakeWifiNai(nc);
+
+        verifyConnectivityDiagnosticsPermissionsWithNetworkAgentInfo(naiWithUid,
+                false /* expectPermission */);
     }
 
     @Test