ConnectivityManager: Address review comments from aosp/1595396

Bug: 156867433
Test: atest android.net
Test: atest com.android.server
Change-Id: I7f5d043732ae22edd14bf581b7dc676c9236b545

framework/src/android/net/ConnectivityManager.java

@@ -3409,6 +3409,8 @@ public class ConnectivityManager {
          * not include location sensitive info.
          * </p>
          */
+        // Note: Some existing fields which are location sensitive may still be included without
+        // this flag if the app targets SDK < S (to maintain backwards compatibility).
         public static final int FLAG_INCLUDE_LOCATION_INFO = 1 << 0;
 
         /** @hide */

framework/src/android/net/NetworkCapabilities.java

@@ -1141,7 +1141,9 @@ public final class NetworkCapabilities implements Parcelable {
      * app needs to hold {@link android.Manifest.permission#ACCESS_FINE_LOCATION} permission. If the
      * app targets SDK version greater than or equal to {@link Build.VERSION_CODES#S}, then they
      * also need to use {@link NetworkCallback#FLAG_INCLUDE_LOCATION_INFO} to get the info in their
-     * callback. The app will be blamed for location access if this field is included.
+     * callback. If the apps targets SDK version equal to {{@link Build.VERSION_CODES#R}, this field
+     * will always be included. The app will be blamed for location access if this field is
+     * included.
      * </p>
      */
     public int getOwnerUid() {

services/core/java/com/android/server/ConnectivityService.java

@@ -5592,7 +5592,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
                     + mNetworkRequestForCallback.requestId
                     + " " + mRequests
                     + (mPendingIntent == null ? "" : " to trigger " + mPendingIntent)
-                    + "callback flags: " + mCallbackFlags;
+                    + " callback flags: " + mCallbackFlags;
         }
     }
 

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

@@ -8978,7 +8978,7 @@ public class ConnectivityServiceTest {
 
         final int expectedOwnerUidWithoutIncludeFlag =
                 shouldInclLocationSensitiveOwnerUidWithoutIncludeFlag
-                        ? Process.myUid() : INVALID_UID;
+                        ? myUid : INVALID_UID;
         assertEquals(expectedOwnerUidWithoutIncludeFlag, getOwnerUidNetCapsPermission(
                 myUid, myUid, false /* includeLocationSensitiveInfo */));
 
@@ -8997,22 +8997,26 @@ public class ConnectivityServiceTest {
 
     }
 
+    private void verifyOwnerUidAndTransportInfoNetCapsPermissionPreS() {
+        verifyOwnerUidAndTransportInfoNetCapsPermission(
+                // Ensure that owner uid is included even if the request asks to remove it (which is
+                // the default) since the app has necessary permissions and targetSdk < S.
+                true, /* shouldInclLocationSensitiveOwnerUidWithoutIncludeFlag */
+                true, /* shouldInclLocationSensitiveOwnerUidWithIncludeFlag */
+                // Ensure that location info is removed if the request asks to remove it even if the
+                // app has necessary permissions.
+                false, /* shouldInclLocationSensitiveTransportInfoWithoutIncludeFlag */
+                true /* shouldInclLocationSensitiveTransportInfoWithIncludeFlag */
+        );
+    }
+
     @Test
-    public void testCreateWithLocationInfoSanitizedWithFineLocationAfterQ()
+    public void testCreateWithLocationInfoSanitizedWithFineLocationAfterQPreS()
             throws Exception {
         setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_FINE_LOCATION,
                 Manifest.permission.ACCESS_FINE_LOCATION);
 
-        verifyOwnerUidAndTransportInfoNetCapsPermission(
-                // Ensure that we include owner uid even if the request asks to remove it since the
-                // app has necessary permissions and targetSdk < S.
-                true, /* shouldInclLocationSensitiveOwnerUidWithoutIncludeFlag */
-                true, /* shouldInclLocationSensitiveOwnerUidWithIncludeFlag */
-                false, /* shouldInclLocationSensitiveTransportInfoWithoutIncludeFlag */
-                // Ensure that we remove location info if the request asks to remove it even if the
-                // app has necessary permissions.
-                true /* shouldInclLocationSensitiveTransportInfoWithIncludeFlag */
-        );
+        verifyOwnerUidAndTransportInfoNetCapsPermissionPreS();
     }
 
     @Test
@@ -9021,16 +9025,7 @@ public class ConnectivityServiceTest {
         setupLocationPermissions(Build.VERSION_CODES.R, true, AppOpsManager.OPSTR_FINE_LOCATION,
                 Manifest.permission.ACCESS_FINE_LOCATION);
 
-        verifyOwnerUidAndTransportInfoNetCapsPermission(
-                // Ensure that we include owner uid even if the request asks to remove it since the
-                // app has necessary permissions and targetSdk < S.
-                true, /* shouldInclLocationSensitiveOwnerUidWithoutIncludeFlag */
-                true, /* shouldInclLocationSensitiveOwnerUidWithIncludeFlag */
-                false, /* shouldInclLocationSensitiveTransportInfoWithoutIncludeFlag */
-                // Ensure that we remove location info if the request asks to remove it even if the
-                // app has necessary permissions.
-                true /* shouldInclLocationSensitiveTransportInfoWithIncludeFlag */
-        );
+        verifyOwnerUidAndTransportInfoNetCapsPermissionPreS();
     }
 
     @Test
@@ -9041,13 +9036,13 @@ public class ConnectivityServiceTest {
                 Manifest.permission.ACCESS_FINE_LOCATION);
 
         verifyOwnerUidAndTransportInfoNetCapsPermission(
-                // Ensure that we owner UID if the request asks us to remove it even if the app
-                // has necessary permissions since targetSdk >= S.
+                // Ensure that the owner UID is removed if the request asks us to remove it even
+                // if the app has necessary permissions since targetSdk >= S.
                 false, /* shouldInclLocationSensitiveOwnerUidWithoutIncludeFlag */
                 true, /* shouldInclLocationSensitiveOwnerUidWithIncludeFlag */
-                false, /* shouldInclLocationSensitiveTransportInfoWithoutIncludeFlag */
-                // Ensure that we remove location info if the request asks to remove it even if the
+                // Ensure that location info is removed if the request asks to remove it even if the
                 // app has necessary permissions.
+                false, /* shouldInclLocationSensitiveTransportInfoWithoutIncludeFlag */
                 true /* shouldInclLocationSensitiveTransportInfoWithIncludeFlag */
         );
     }
@@ -9058,15 +9053,15 @@ public class ConnectivityServiceTest {
         setupLocationPermissions(Build.VERSION_CODES.P, true, AppOpsManager.OPSTR_COARSE_LOCATION,
                 Manifest.permission.ACCESS_COARSE_LOCATION);
 
+        verifyOwnerUidAndTransportInfoNetCapsPermissionPreS();
+    }
+
+    private void verifyOwnerUidAndTransportInfoNetCapsNotIncluded() {
         verifyOwnerUidAndTransportInfoNetCapsPermission(
-                // Ensure that we owner UID if the request asks us to remove it even if the app
-                // has necessary permissions since targetSdk >= S.
-                true, /* shouldInclLocationSensitiveOwnerUidWithoutIncludeFlag */
-                true, /* shouldInclLocationSensitiveOwnerUidWithIncludeFlag */
+                false, /* shouldInclLocationSensitiveOwnerUidWithoutIncludeFlag */
+                false, /* shouldInclLocationSensitiveOwnerUidWithIncludeFlag */
                 false, /* shouldInclLocationSensitiveTransportInfoWithoutIncludeFlag */
-                // Ensure that we remove location info if the request asks to remove it even if the
-                // app has necessary permissions.
-                true /* shouldInclLocationSensitiveTransportInfoWithIncludeFlag */
+                false /* shouldInclLocationSensitiveTransportInfoWithIncludeFlag */
         );
     }
 
@@ -9076,12 +9071,7 @@ public class ConnectivityServiceTest {
         setupLocationPermissions(Build.VERSION_CODES.Q, false, AppOpsManager.OPSTR_FINE_LOCATION,
                 Manifest.permission.ACCESS_FINE_LOCATION);
 
-        verifyOwnerUidAndTransportInfoNetCapsPermission(
-                false, /* shouldInclLocationSensitiveOwnerUidWithoutIncludeFlag */
-                false, /* shouldInclLocationSensitiveOwnerUidWithIncludeFlag */
-                false, /* shouldInclLocationSensitiveTransportInfoWithoutIncludeFlag */
-                false /* shouldInclLocationSensitiveTransportInfoWithIncludeFlag */
-        );
+        verifyOwnerUidAndTransportInfoNetCapsNotIncluded();
     }
 
     @Test
@@ -9103,26 +9093,17 @@ public class ConnectivityServiceTest {
         setupLocationPermissions(Build.VERSION_CODES.Q, true, AppOpsManager.OPSTR_COARSE_LOCATION,
                 Manifest.permission.ACCESS_COARSE_LOCATION);
 
-        verifyOwnerUidAndTransportInfoNetCapsPermission(
-                false, /* shouldInclLocationSensitiveOwnerUidWithoutIncludeFlag */
-                false, /* shouldInclLocationSensitiveOwnerUidWithIncludeFlag */
-                false, /* shouldInclLocationSensitiveTransportInfoWithoutIncludeFlag */
-                false /* shouldInclLocationSensitiveTransportInfoWithIncludeFlag */
-        );
+        verifyOwnerUidAndTransportInfoNetCapsNotIncluded();
     }
 
     @Test
-    public void testCreateWithLocationInfoSanitizedWithoutLocationPermission()
+    public void testCreateWithLocationInfoSanitizedWithCoarseLocationAfterS()
             throws Exception {
         // Test that not having fine location permission leads to sanitization.
-        setupLocationPermissions(Build.VERSION_CODES.Q, true, null /* op */, null /* perm */);
+        setupLocationPermissions(Build.VERSION_CODES.S, true, AppOpsManager.OPSTR_COARSE_LOCATION,
+                Manifest.permission.ACCESS_COARSE_LOCATION);
 
-        verifyOwnerUidAndTransportInfoNetCapsPermission(
-                false, /* shouldInclLocationSensitiveOwnerUidWithoutIncludeFlag */
-                false, /* shouldInclLocationSensitiveOwnerUidWithIncludeFlag */
-                false, /* shouldInclLocationSensitiveTransportInfoWithoutIncludeFlag */
-                false /* shouldInclLocationSensitiveTransportInfoWithIncludeFlag */
-        );
+        verifyOwnerUidAndTransportInfoNetCapsNotIncluded();
     }
 
     @Test