Merge "CarrierConfigLoader: IPC call with invalid subId will silently fail"
diff --git a/src/com/android/phone/CarrierConfigLoader.java b/src/com/android/phone/CarrierConfigLoader.java
index 3d9e0b0..76a16d3 100644
--- a/src/com/android/phone/CarrierConfigLoader.java
+++ b/src/com/android/phone/CarrierConfigLoader.java
@@ -1178,20 +1178,20 @@
@Override
@NonNull
- public PersistableBundle getConfigForSubId(int subId, String callingPackage) {
- return getConfigForSubIdWithFeature(subId, callingPackage, null);
+ public PersistableBundle getConfigForSubId(int subscriptionId, String callingPackage) {
+ return getConfigForSubIdWithFeature(subscriptionId, callingPackage, null);
}
@Override
@NonNull
- public PersistableBundle getConfigForSubIdWithFeature(int subId, String callingPackage,
+ public PersistableBundle getConfigForSubIdWithFeature(int subscriptionId, String callingPackage,
String callingFeatureId) {
- if (!TelephonyPermissions.checkCallingOrSelfReadPhoneState(mContext, subId, callingPackage,
- callingFeatureId, "getCarrierConfig")) {
+ if (!TelephonyPermissions.checkCallingOrSelfReadPhoneState(mContext, subscriptionId,
+ callingPackage, callingFeatureId, "getCarrierConfig")) {
return new PersistableBundle();
}
- int phoneId = SubscriptionManager.getPhoneId(subId);
+ int phoneId = SubscriptionManager.getPhoneId(subscriptionId);
PersistableBundle retConfig = CarrierConfigManager.getDefaultConfig();
if (SubscriptionManager.isValidPhoneId(phoneId)) {
PersistableBundle config = mConfigFromDefaultApp[phoneId];
@@ -1235,7 +1235,8 @@
int phoneId = SubscriptionManager.getPhoneId(subscriptionId);
if (!SubscriptionManager.isValidPhoneId(phoneId)) {
logd("Ignore invalid phoneId: " + phoneId + " for subId: " + subscriptionId);
- return;
+ throw new IllegalArgumentException(
+ "Invalid phoneId " + phoneId + " for subId " + subscriptionId);
}
// Post to run on handler thread on which all states should be confined.
mHandler.post(() -> {
@@ -1274,17 +1275,18 @@
}
@Override
- public void notifyConfigChangedForSubId(int subId) {
- int phoneId = SubscriptionManager.getPhoneId(subId);
- if (!SubscriptionManager.isValidPhoneId(phoneId)) {
- logd("Ignore invalid phoneId: " + phoneId + " for subId: " + subId);
- return;
- }
-
+ public void notifyConfigChangedForSubId(int subscriptionId) {
// Requires the calling app to be either a carrier privileged app for this subId or
// system privileged app with MODIFY_PHONE_STATE permission.
- TelephonyPermissions.enforceCallingOrSelfModifyPermissionOrCarrierPrivilege(mContext, subId,
- "Require carrier privileges or MODIFY_PHONE_STATE permission.");
+ TelephonyPermissions.enforceCallingOrSelfModifyPermissionOrCarrierPrivilege(mContext,
+ subscriptionId, "Require carrier privileges or MODIFY_PHONE_STATE permission.");
+
+ int phoneId = SubscriptionManager.getPhoneId(subscriptionId);
+ if (!SubscriptionManager.isValidPhoneId(phoneId)) {
+ logd("Ignore invalid phoneId: " + phoneId + " for subId: " + subscriptionId);
+ throw new IllegalArgumentException(
+ "Invalid phoneId " + phoneId + " for subId " + subscriptionId);
+ }
// This method should block until deleting has completed, so that an error which prevents us
// from clearing the cache is passed back to the carrier app. With the files successfully
@@ -1301,7 +1303,7 @@
android.Manifest.permission.MODIFY_PHONE_STATE, null);
logdWithLocalLog("Update config for phoneId: " + phoneId + " simState: " + simState);
if (!SubscriptionManager.isValidPhoneId(phoneId)) {
- return;
+ throw new IllegalArgumentException("Invalid phoneId: " + phoneId);
}
// requires Java 7 for switch on string.
switch (simState) {
diff --git a/tests/src/com/android/phone/CarrierConfigLoaderTest.java b/tests/src/com/android/phone/CarrierConfigLoaderTest.java
index b7d1d70..f58e6cc 100644
--- a/tests/src/com/android/phone/CarrierConfigLoaderTest.java
+++ b/tests/src/com/android/phone/CarrierConfigLoaderTest.java
@@ -28,7 +28,6 @@
import static org.mockito.ArgumentMatchers.nullable;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import android.content.Intent;
@@ -149,6 +148,20 @@
}
/**
+ * Verifies that IllegalArgumentException should throw when call #updateConfigForPhoneId() with
+ * invalid phoneId.
+ */
+ @Test
+ public void testUpdateConfigForPhoneId_invalidPhoneId() throws Exception {
+ mContext.grantPermission(STUB_PERMISSION_ENABLE_ALL);
+
+ assertThrows(IllegalArgumentException.class,
+ () -> mCarrierConfigLoader.updateConfigForPhoneId(
+ SubscriptionManager.INVALID_PHONE_INDEX,
+ IccCardConstants.INTENT_VALUE_ICC_ABSENT));
+ }
+
+ /**
* Verifies that when call #updateConfigForPhoneId() with SIM absence, both carrier config from
* default app and carrier should be cleared but no-sim config should be loaded.
*/
@@ -219,6 +232,17 @@
}
/**
+ * Verifies IllegalArgumentException should throw if call #overrideConfig() with invalid subId.
+ */
+ @Test
+ public void testOverrideConfig_invalidSubId() throws Exception {
+ mContext.grantPermission(STUB_PERMISSION_ENABLE_ALL);
+
+ assertThrows(IllegalArgumentException.class, () -> mCarrierConfigLoader.overrideConfig(
+ SubscriptionManager.INVALID_SUBSCRIPTION_ID, new PersistableBundle(), false));
+ }
+
+ /**
* Verifies that override config is not null when calling #overrideConfig with null bundle.
*/
@Test
@@ -263,20 +287,16 @@
}
/**
- * Verifies that calling #notifyConfigChangedForSubId() with invalid subId will be ignored.
+ * Verifies that IllegalArgumentException should throw when calling
+ * #notifyConfigChangedForSubId() with invalid subId.
*/
@Test
public void testNotifyConfigChangedForSubId_invalidSubId() throws Exception {
mContext.grantPermission(STUB_PERMISSION_ENABLE_ALL);
- mCarrierConfigLoader.notifyConfigChangedForSubId(
- SubscriptionManager.INVALID_SUBSCRIPTION_ID);
-
- // verifying the behavior following the permission check is not actually performed.
- // It against "verify value instead of behavior", but we don't have other alternatives.
- verify(mPackageManager, never()).getNameForUid(anyInt());
- verify(mContext, never()).checkCallingOrSelfPermission(
- eq(android.Manifest.permission.MODIFY_PHONE_STATE));
+ assertThrows(IllegalArgumentException.class,
+ () -> mCarrierConfigLoader.notifyConfigChangedForSubId(
+ SubscriptionManager.INVALID_SUBSCRIPTION_ID));
}
// TODO(b/184040111): Enable test case when support disabling carrier privilege