Use platform-compat-fw to separate enforce location change
The location permission check enforcement introduced in b/182384053
is separated with CompatChanges#isChangeEnabled to keep better
compatibility.
Bug: 191911306
Test: atest ServiceStateProviderTest
Change-Id: I3d9b24e05e86cbcd7f64d9c0969171d7db1c8756
diff --git a/Android.bp b/Android.bp
index 4357f12..f3c9ebc 100644
--- a/Android.bp
+++ b/Android.bp
@@ -40,6 +40,7 @@
"voip-common",
"ims-common",
"libprotobuf-java-lite",
+ "app-compat-annotations",
"unsupportedappusage",
],
@@ -95,3 +96,8 @@
type: "lite",
},
}
+
+platform_compat_config {
+ name: "TeleService-platform-compat-config",
+ src: ":TeleService",
+}
diff --git a/AndroidManifest.xml b/AndroidManifest.xml
index 66a2422..61aa975 100644
--- a/AndroidManifest.xml
+++ b/AndroidManifest.xml
@@ -231,6 +231,9 @@
<uses-permission android:name="android.permission.MANAGE_SUBSCRIPTION_PLANS"/>
<uses-permission android:name="android.permission.OBSERVE_ROLE_HOLDERS"/>
<uses-permission android:name="android.permission.BIND_GBA_SERVICE"/>
+ <!-- Permissions required for reading and logging compat changes -->
+ <uses-permission android:name="android.permission.LOG_COMPAT_CHANGE"/>
+ <uses-permission android:name="android.permission.READ_COMPAT_CHANGE_CONFIG"/>
<application android:name="PhoneApp"
android:persistent="true"
diff --git a/src/com/android/phone/ServiceStateProvider.java b/src/com/android/phone/ServiceStateProvider.java
index 56786f9..6f5fcf5 100644
--- a/src/com/android/phone/ServiceStateProvider.java
+++ b/src/com/android/phone/ServiceStateProvider.java
@@ -28,6 +28,9 @@
import static android.provider.Telephony.ServiceStateTable.getUriForSubscriptionIdAndField;
import android.Manifest;
+import android.app.compat.CompatChanges;
+import android.compat.annotation.ChangeId;
+import android.compat.annotation.EnabledAfter;
import android.content.ContentProvider;
import android.content.ContentValues;
import android.content.Context;
@@ -230,6 +233,15 @@
*/
public static final String OPERATOR_ALPHA_SHORT_RAW = "operator_alpha_short_raw";
+ /**
+ * If the change Id is enabled, location permission is required to access location sensitive
+ * columns in the ServiceStateTable.
+ */
+ @ChangeId
+ @EnabledAfter(targetSdkVersion = Build.VERSION_CODES.R)
+ @VisibleForTesting
+ /* package */ static final long ENFORCE_LOCATION_PERMISSION_CHECK = 191911306;
+
private final HashMap<Integer, ServiceState> mServiceStates = new HashMap<>();
@VisibleForTesting
@@ -398,7 +410,8 @@
return null;
}
- // TODO(b/182384053): replace targetSdk check with CompatChanges#isChangeEnabled
+ final boolean enforceLocationPermission =
+ CompatChanges.isChangeEnabled(ENFORCE_LOCATION_PERMISSION_CHECK);
final boolean targetingAtLeastS = TelephonyPermissions.getTargetSdk(getContext(),
getCallingPackage()) >= Build.VERSION_CODES.S;
final boolean canReadPrivilegedPhoneState = getContext().checkCallingOrSelfPermission(
@@ -406,7 +419,7 @@
final String[] availableColumns;
final ServiceState ss;
- if (targetingAtLeastS && !canReadPrivilegedPhoneState) {
+ if (enforceLocationPermission && targetingAtLeastS && !canReadPrivilegedPhoneState) {
// targetSdkVersion S+ without read privileged phone state permission can only
// access public columns which have no location sensitive info.
availableColumns = PUBLIC_COLUMNS;
@@ -415,9 +428,9 @@
availableColumns = ALL_COLUMNS;
final boolean hasLocationPermission = hasLocationPermission();
- if (hasLocationPermission) {
+ if (!enforceLocationPermission || hasLocationPermission) {
// No matter the targetSdkVersion, return unredacted ServiceState if caller does
- // have location permission.
+ // have location permission or location permission enforcement is not introduced
ss = unredactedServiceState;
} else {
// The caller has targetSdkVersion S+ but no location permission. It explicitly
diff --git a/tests/Android.bp b/tests/Android.bp
index c180476..083d64d 100644
--- a/tests/Android.bp
+++ b/tests/Android.bp
@@ -50,7 +50,8 @@
"telephony-common-testing",
"testng",
"truth-prebuilt",
- "testables",
+ "testables",
+ "platform-compat-test-rules",
],
test_suites: [
diff --git a/tests/src/com/android/phone/ServiceStateProviderTest.java b/tests/src/com/android/phone/ServiceStateProviderTest.java
index cde584f..532b1c0 100644
--- a/tests/src/com/android/phone/ServiceStateProviderTest.java
+++ b/tests/src/com/android/phone/ServiceStateProviderTest.java
@@ -27,6 +27,8 @@
import static android.provider.Telephony.ServiceStateTable.getUriForSubscriptionId;
import static android.telephony.NetworkRegistrationInfo.REGISTRATION_STATE_HOME;
+import static com.android.phone.ServiceStateProvider.ENFORCE_LOCATION_PERMISSION_CHECK;
+
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
@@ -42,6 +44,7 @@
import android.Manifest;
import android.app.AppOpsManager;
+import android.compat.testing.PlatformCompatChangeRule;
import android.content.Context;
import android.content.pm.ApplicationInfo;
import android.content.pm.PackageManager;
@@ -62,10 +65,14 @@
import androidx.test.ext.junit.runners.AndroidJUnit4;
+import libcore.junit.util.compat.CoreCompatChangeRule;
+
import org.junit.After;
import org.junit.Before;
import org.junit.Ignore;
+import org.junit.Rule;
import org.junit.Test;
+import org.junit.rules.TestRule;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
@@ -91,6 +98,9 @@
@Mock LocationManager mLocationManager;
@Mock PackageManager mPackageManager;
+ @Rule
+ public TestRule compatChangeRule = new PlatformCompatChangeRule();
+
// Exception used internally to verify if the Resolver#notifyChange has been called.
private class TestNotifierException extends RuntimeException {
TestNotifierException() {
@@ -170,6 +180,7 @@
// TODO(b/191995565): Turn this on when location access can be off
@Ignore
@SmallTest
+ @CoreCompatChangeRule.EnableCompatChanges({ENFORCE_LOCATION_PERMISSION_CHECK})
public void testQueryServiceState_withNoSubId_withoutLocation() {
setLocationPermissions(false);
@@ -239,7 +250,19 @@
* public columns of ServiceStateTable.
*/
@Test
- public void query_publicColumns_targetS_noReadPrivilege_getPublicColumns() {
+ @CoreCompatChangeRule.EnableCompatChanges({ENFORCE_LOCATION_PERMISSION_CHECK})
+ public void
+ query_publicColumns_enforceLocatoinEnabled_targetS_noReadPrivilege_getPublicColumns() {
+ setTargetSdkVersion(Build.VERSION_CODES.S);
+ setCanReadPrivilegedPhoneState(false);
+
+ verifyServiceStateWithPublicColumns(mTestServiceState, null /*projection*/);
+ }
+
+ @Test
+ @CoreCompatChangeRule.DisableCompatChanges({ENFORCE_LOCATION_PERMISSION_CHECK})
+ public void
+ query_publicColumns_enforceLocationDisabled_targetS_noReadPrivilege_getPublicColumns() {
setTargetSdkVersion(Build.VERSION_CODES.S);
setCanReadPrivilegedPhoneState(false);
@@ -251,6 +274,7 @@
* non-public columns should throw IllegalArgumentException.
*/
@Test
+ @CoreCompatChangeRule.EnableCompatChanges({ENFORCE_LOCATION_PERMISSION_CHECK})
public void query_hideColumn_targetS_noReadPrivilege_throwIllegalArgumentException() {
setTargetSdkVersion(Build.VERSION_CODES.S);
setCanReadPrivilegedPhoneState(false);
@@ -263,11 +287,30 @@
}
/**
- * Verify that apps target S+ with READ_PRIVILEGED_PHONE_STATE and location permissions should
- * be able to access all columns.
+ * Verify that with changeId ENFORCE_LOCATION_PERMISSION_CHECK enabled, apps target S+ with
+ * READ_PRIVILEGED_PHONE_STATE and location permissions should be able to access all columns.
*/
@Test
- public void query_allColumn_targetS_withReadPrivilegedAndLocation_getAllStateUnredacted() {
+ @CoreCompatChangeRule.EnableCompatChanges({ENFORCE_LOCATION_PERMISSION_CHECK})
+ public void
+ query_allColumn_enforceLocationEnabled_targetS_withReadPrivilegedAndLocation_getUnredacted() {
+ setTargetSdkVersion(Build.VERSION_CODES.S);
+ setCanReadPrivilegedPhoneState(true);
+ setLocationPermissions(true);
+
+ verifyServiceStateForSubId(
+ getUriForSubscriptionId(SubscriptionManager.DEFAULT_SUBSCRIPTION_ID),
+ mTestServiceState, true /*hasPermission*/);
+ }
+
+ /**
+ * Verify that with changeId ENFORCE_LOCATION_PERMISSION_CHECK disabled, apps target S+ with
+ * READ_PRIVILEGED_PHONE_STATE and location permissions should be able to access all columns.
+ */
+ @Test
+ @CoreCompatChangeRule.DisableCompatChanges({ENFORCE_LOCATION_PERMISSION_CHECK})
+ public void
+ query_allColumn_enforceLocationDisabled_targetS_withReadPrivilegedAndLocation_getUnredacted() {
setTargetSdkVersion(Build.VERSION_CODES.S);
setCanReadPrivilegedPhoneState(true);
setLocationPermissions(true);
@@ -283,6 +326,7 @@
*/
// TODO(b/191995565): Turn this on once b/191995565 is integrated
@Ignore
+ @CoreCompatChangeRule.EnableCompatChanges({ENFORCE_LOCATION_PERMISSION_CHECK})
public void query_locationColumn_targetS_withReadPrivilegeNoLocation_throwSecurityExecption() {
setTargetSdkVersion(Build.VERSION_CODES.S);
setCanReadPrivilegedPhoneState(true);
@@ -296,10 +340,12 @@
}
/**
- * Verify that apps target R- with location permissions should be able to access all columns.
+ * Verify that when changeId ENFORCE_LOCATION_PERMISSION_CHECK is enabled, apps target R- with
+ * location permissions should be able to access all columns.
*/
@Test
- public void query_allColumn_targetR_withLocation_getAllStateUnredacted() {
+ @CoreCompatChangeRule.EnableCompatChanges({ENFORCE_LOCATION_PERMISSION_CHECK})
+ public void query_allColumn_enforceLoationEnabled_targetR_withLocation_getUnredacted() {
setTargetSdkVersion(Build.VERSION_CODES.R);
setLocationPermissions(true);
@@ -309,12 +355,28 @@
}
/**
- * Verify that apps target R- w/o location permissions should be able to access all columns but
- * with redacted ServiceState.
+ * Verify that when changeId ENFORCE_LOCATION_PERMISSION_CHECK is disabled, apps target R- with
+ * location permissions should be able to access all columns.
+ */
+ @Test
+ @CoreCompatChangeRule.DisableCompatChanges({ENFORCE_LOCATION_PERMISSION_CHECK})
+ public void query_allColumn_enforceLocationDisabled_targetR_withLocation_getUnredacted() {
+ setTargetSdkVersion(Build.VERSION_CODES.R);
+ setLocationPermissions(true);
+
+ verifyServiceStateForSubId(
+ getUriForSubscriptionId(SubscriptionManager.DEFAULT_SUBSCRIPTION_ID),
+ mTestServiceState, true /*hasPermission*/);
+ }
+
+ /**
+ * Verify that changeId ENFORCE_LOCATION_PERMISSION_CHECK is enabled, apps target R- w/o
+ * location permissions should be able to access all columns but with redacted ServiceState.
*/
// TODO(b/191995565): Turn case on when location access can be off
@Ignore
- public void query_allColumn_targetR_noLocation_getRedacted() {
+ @CoreCompatChangeRule.EnableCompatChanges({ENFORCE_LOCATION_PERMISSION_CHECK})
+ public void query_allColumn_enforceLocationEnabled_targetR_noLocation_getRedacted() {
setTargetSdkVersion(Build.VERSION_CODES.R);
setLocationPermissions(false);
@@ -324,6 +386,22 @@
true /*hasPermission*/);
}
+ /**
+ * Verify that changeId ENFORCE_LOCATION_PERMISSION_CHECK is disabled, apps target R- w/o
+ * location permissions should be able to access all columns and with unredacted ServiceState.
+ */
+ // TODO(b/191995565): Turn case on when location access can be off
+ @Ignore
+ @CoreCompatChangeRule.DisableCompatChanges({ENFORCE_LOCATION_PERMISSION_CHECK})
+ public void query_allColumn_enforceLocationDisabled_targetR_noLocation_getUnredacted() {
+ setTargetSdkVersion(Build.VERSION_CODES.R);
+ setLocationPermissions(false);
+
+ verifyServiceStateForSubId(
+ getUriForSubscriptionId(SubscriptionManager.DEFAULT_SUBSCRIPTION_ID),
+ mTestServiceState, true /*hasPermission*/);
+ }
+
private void verifyServiceStateWithLocationColumns(ServiceState ss, String[] projection) {
try (Cursor cursor = mContentResolver.query(ServiceStateTable.CONTENT_URI, projection, null,
null)) {
@@ -335,7 +413,6 @@
try (Cursor cursor = mContentResolver.query(ServiceStateTable.CONTENT_URI, projection, null,
null)) {
assertNotNull(cursor);
- assertEquals(cursor.getColumnCount(), ServiceStateProvider.PUBLIC_COLUMNS.length);
cursor.moveToFirst();
assertEquals(ss.getVoiceRegState(),