Merge "Revert "CarrierConfigLoader: IPC call with invalid subId will silently fail"" am: d25c09b3bd am: fc65d9ed3a am: 705c648eef
Original change: https://android-review.googlesource.com/c/platform/packages/services/Telephony/+/1668365
Change-Id: I8ba6bb112f93403584878fb424486639cdf8e539
diff --git a/src/com/android/phone/CarrierConfigLoader.java b/src/com/android/phone/CarrierConfigLoader.java
index b4a01a8..e729d28 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 subscriptionId, String callingPackage) {
- return getConfigForSubIdWithFeature(subscriptionId, callingPackage, null);
+ public PersistableBundle getConfigForSubId(int subId, String callingPackage) {
+ return getConfigForSubIdWithFeature(subId, callingPackage, null);
}
@Override
@NonNull
- public PersistableBundle getConfigForSubIdWithFeature(int subscriptionId, String callingPackage,
+ public PersistableBundle getConfigForSubIdWithFeature(int subId, String callingPackage,
String callingFeatureId) {
- if (!TelephonyPermissions.checkCallingOrSelfReadPhoneState(mContext, subscriptionId,
- callingPackage, callingFeatureId, "getCarrierConfig")) {
+ if (!TelephonyPermissions.checkCallingOrSelfReadPhoneState(mContext, subId, callingPackage,
+ callingFeatureId, "getCarrierConfig")) {
return PersistableBundle.EMPTY;
}
- int phoneId = SubscriptionManager.getPhoneId(subscriptionId);
+ int phoneId = SubscriptionManager.getPhoneId(subId);
PersistableBundle retConfig = CarrierConfigManager.getDefaultConfig();
if (SubscriptionManager.isValidPhoneId(phoneId)) {
PersistableBundle config = mConfigFromDefaultApp[phoneId];
@@ -1235,8 +1235,7 @@
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);
+ return;
}
// Post to run on handler thread on which all states should be confined.
mHandler.post(() -> {
@@ -1275,18 +1274,17 @@
}
@Override
- public void notifyConfigChangedForSubId(int subscriptionId) {
+ public void notifyConfigChangedForSubId(int subId) {
+ int phoneId = SubscriptionManager.getPhoneId(subId);
+ if (!SubscriptionManager.isValidPhoneId(phoneId)) {
+ logd("Ignore invalid phoneId: " + phoneId + " for subId: " + subId);
+ return;
+ }
+
// 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,
- 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);
- }
+ TelephonyPermissions.enforceCallingOrSelfModifyPermissionOrCarrierPrivilege(mContext, subId,
+ "Require carrier privileges or MODIFY_PHONE_STATE permission.");
// 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
@@ -1303,7 +1301,7 @@
android.Manifest.permission.MODIFY_PHONE_STATE, null);
logdWithLocalLog("Update config for phoneId: " + phoneId + " simState: " + simState);
if (!SubscriptionManager.isValidPhoneId(phoneId)) {
- throw new IllegalArgumentException("Invalid phoneId: " + phoneId);
+ return;
}
// 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 d2c484c..7fd9069 100644
--- a/tests/src/com/android/phone/CarrierConfigLoaderTest.java
+++ b/tests/src/com/android/phone/CarrierConfigLoaderTest.java
@@ -28,6 +28,7 @@
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;
@@ -148,20 +149,6 @@
}
/**
- * 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.
*/
@@ -232,17 +219,6 @@
}
/**
- * 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
@@ -288,16 +264,20 @@
}
/**
- * Verifies that IllegalArgumentException should throw when calling
- * #notifyConfigChangedForSubId() with invalid subId.
+ * Verifies that calling #notifyConfigChangedForSubId() with invalid subId will be ignored.
*/
@Test
public void testNotifyConfigChangedForSubId_invalidSubId() throws Exception {
mContext.grantPermission(STUB_PERMISSION_ENABLE_ALL);
- assertThrows(IllegalArgumentException.class,
- () -> mCarrierConfigLoader.notifyConfigChangedForSubId(
- SubscriptionManager.INVALID_SUBSCRIPTION_ID));
+ 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));
}
// TODO(b/184040111): Enable test case when support disabling carrier privilege