From f75ffaaded974761555aaaea6f6c72915294c05d Mon Sep 17 00:00:00 2001 From: Roshan Pius Date: Thu, 11 Mar 2021 21:16:44 -0800 Subject: [PATCH] ConnectivityManager: Address review comments from aosp/1595396 Bug: 156867433 Test: atest android.net Test: atest com.android.server Change-Id: I7f5d043732ae22edd14bf581b7dc676c9236b545 --- .../src/android/net/ConnectivityManager.java | 2 + .../src/android/net/NetworkCapabilities.java | 4 +- .../android/server/ConnectivityService.java | 2 +- .../server/ConnectivityServiceTest.java | 87 ++++++++----------- 4 files changed, 40 insertions(+), 55 deletions(-) diff --git a/framework/src/android/net/ConnectivityManager.java b/framework/src/android/net/ConnectivityManager.java index d196c1a2d1..68f9fbbd28 100644 --- a/framework/src/android/net/ConnectivityManager.java +++ b/framework/src/android/net/ConnectivityManager.java @@ -3409,6 +3409,8 @@ public class ConnectivityManager { * not include location sensitive info. *

*/ + // 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 */ diff --git a/framework/src/android/net/NetworkCapabilities.java b/framework/src/android/net/NetworkCapabilities.java index 881fa8c270..d9acc3178a 100644 --- a/framework/src/android/net/NetworkCapabilities.java +++ b/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. *

*/ public int getOwnerUid() { diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 0c4258561f..533196466d 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/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; } } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 4c0c1198a6..af5668f4ef 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/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