Make getPhoneNumber methods safer and faster
Make the getPhoneNumber() methods a little bit safer
and easier to work with by having both APIs call a
single internal utility method. This eliminates redundant
permissions checks that could all result from a single
method call to getPhoneNumber. In very rare cases, these
permissions checks could have resulted in inconsistent
results due to context switching.
Also fix an issue where DEFAULT_SUBSCRIPTION_ID had some
inconsistent behavior. It would work "correctly" for
PHONE_NUMBER_SOURCE_UICC but wouldn't for the other two
sources.
Bug: 317673478
Test: atest FrameworksTelephonyTests
Change-Id: I53fe0e7afefe1f2bc7d280fa74768b77962bc078
diff --git a/flags/subscription.aconfig b/flags/subscription.aconfig
index 60c0af1..32e8f2d 100644
--- a/flags/subscription.aconfig
+++ b/flags/subscription.aconfig
@@ -52,4 +52,16 @@
namespace: "telephony"
description: "Supports querying if a subscription is associated with the caller"
bug: "325045841"
-}
\ No newline at end of file
+}
+
+# OWNER=nharold TARGET=24Q3
+flag {
+ name: "safer_get_phone_number"
+ namespace: "telephony"
+ description: "Safety and performance improvements for getPhoneNumber()"
+ bug: "317673478"
+
+ metadata {
+ purpose: PURPOSE_BUGFIX
+ }
+}
diff --git a/src/java/com/android/internal/telephony/subscription/SubscriptionManagerService.java b/src/java/com/android/internal/telephony/subscription/SubscriptionManagerService.java
index d96c549..9795dae 100644
--- a/src/java/com/android/internal/telephony/subscription/SubscriptionManagerService.java
+++ b/src/java/com/android/internal/telephony/subscription/SubscriptionManagerService.java
@@ -3712,41 +3712,114 @@
"carrier privileges",
})
public String getPhoneNumber(int subId, @PhoneNumberSource int source,
- @NonNull String callingPackage, @Nullable String callingFeatureId) {
+ @NonNull String callingPackage, @Nullable String callingFeatureId /* unused */) {
TelephonyPermissions.enforceAnyPermissionGrantedOrCarrierPrivileges(
mContext, subId, Binder.getCallingUid(), "getPhoneNumber",
Manifest.permission.READ_PHONE_NUMBERS,
Manifest.permission.READ_PRIVILEGED_PHONE_STATE);
-
enforceTelephonyFeatureWithException(callingPackage, "getPhoneNumber");
- final long identity = Binder.clearCallingIdentity();
- try {
- SubscriptionInfoInternal subInfo = mSubscriptionDatabaseManager
- .getSubscriptionInfoInternal(subId);
+ if (mFeatureFlags.saferGetPhoneNumber()) {
+ checkPhoneNumberSource(source);
+ subId = checkAndGetSubId(subId);
+ if (subId == SubscriptionManager.INVALID_SUBSCRIPTION_ID) return "";
- if (subInfo == null) {
- loge("Invalid sub id " + subId + ", callingPackage=" + callingPackage);
- return "";
+ final long identity = Binder.clearCallingIdentity();
+ try {
+ return getPhoneNumberFromSourceInternal(subId, source);
+ } finally {
+ Binder.restoreCallingIdentity(identity);
}
+ } else {
+ final long identity = Binder.clearCallingIdentity();
+ try {
+ SubscriptionInfoInternal subInfo = mSubscriptionDatabaseManager
+ .getSubscriptionInfoInternal(subId);
- switch(source) {
- case SubscriptionManager.PHONE_NUMBER_SOURCE_UICC:
- Phone phone = PhoneFactory.getPhone(getSlotIndex(subId));
- if (phone != null) {
+ if (subInfo == null) {
+ loge("Invalid sub id " + subId + ", callingPackage=" + callingPackage);
+ return "";
+ }
+
+ switch(source) {
+ case SubscriptionManager.PHONE_NUMBER_SOURCE_UICC:
+ Phone phone = PhoneFactory.getPhone(getSlotIndex(subId));
+ if (phone != null) {
return TextUtils.emptyIfNull(phone.getLine1Number());
- } else {
+ } else {
return subInfo.getNumber();
- }
- case SubscriptionManager.PHONE_NUMBER_SOURCE_CARRIER:
- return subInfo.getNumberFromCarrier();
- case SubscriptionManager.PHONE_NUMBER_SOURCE_IMS:
- return subInfo.getNumberFromIms();
- default:
- throw new IllegalArgumentException("Invalid number source " + source);
+ }
+ case SubscriptionManager.PHONE_NUMBER_SOURCE_CARRIER:
+ return subInfo.getNumberFromCarrier();
+ case SubscriptionManager.PHONE_NUMBER_SOURCE_IMS:
+ return subInfo.getNumberFromIms();
+ default:
+ throw new IllegalArgumentException("Invalid number source " + source);
+ }
+ } finally {
+ Binder.restoreCallingIdentity(identity);
}
- } finally {
- Binder.restoreCallingIdentity(identity);
+ }
+ }
+
+ /**
+ * Get a resolved subId based on what the user passed in.
+ *
+ * Only use this before clearing the calling binder. Used for compatibility (only).
+ * Do not use this behavior for new methods.
+ *
+ * @param subId the subId passed in by the user.
+ */
+ private int checkAndGetSubId(int subId) {
+ if (subId == SubscriptionManager.INVALID_SUBSCRIPTION_ID) {
+ // for historical reasons, INVALID_SUB_ID fails gracefully
+ return subId;
+ } else if (subId == SubscriptionManager.DEFAULT_SUBSCRIPTION_ID) {
+ return getDefaultSubId();
+ } else if (!SubscriptionManager.isValidSubscriptionId(subId)) {
+ throw new IllegalArgumentException("Invalid SubId=" + subId);
+ } else {
+ return subId;
+ }
+ }
+
+ private void checkPhoneNumberSource(int source) {
+ if (source == SubscriptionManager.PHONE_NUMBER_SOURCE_UICC
+ || source == SubscriptionManager.PHONE_NUMBER_SOURCE_CARRIER
+ || source == SubscriptionManager.PHONE_NUMBER_SOURCE_IMS) {
+ return;
+ }
+
+ throw new IllegalArgumentException("Invalid number source " + source);
+ }
+
+ private @NonNull String getPhoneNumberFromSourceInternal(
+ int subId,
+ @PhoneNumberSource int source) {
+
+ final SubscriptionInfoInternal subInfo = mSubscriptionDatabaseManager
+ .getSubscriptionInfoInternal(subId);
+
+ if (subInfo == null) {
+ loge("No SubscriptionInfo found for subId=" + subId);
+ return "";
+ }
+
+ switch(source) {
+ case SubscriptionManager.PHONE_NUMBER_SOURCE_UICC:
+ final Phone phone = PhoneFactory.getPhone(getSlotIndex(subId));
+ if (phone != null) {
+ return TextUtils.emptyIfNull(phone.getLine1Number());
+ } else {
+ return subInfo.getNumber();
+ }
+ case SubscriptionManager.PHONE_NUMBER_SOURCE_CARRIER:
+ return subInfo.getNumberFromCarrier();
+ case SubscriptionManager.PHONE_NUMBER_SOURCE_IMS:
+ return subInfo.getNumberFromIms();
+ default:
+ loge("No SubscriptionInfo found for subId=" + subId);
+ return "";
}
}
@@ -3782,25 +3855,51 @@
enforceTelephonyFeatureWithException(callingPackage,
"getPhoneNumberFromFirstAvailableSource");
- String numberFromCarrier = getPhoneNumber(subId,
- SubscriptionManager.PHONE_NUMBER_SOURCE_CARRIER, callingPackage,
- callingFeatureId);
- if (!TextUtils.isEmpty(numberFromCarrier)) {
- return numberFromCarrier;
+ if (mFeatureFlags.saferGetPhoneNumber()) {
+ subId = checkAndGetSubId(subId);
+ if (subId == SubscriptionManager.INVALID_SUBSCRIPTION_ID) return "";
+
+ final long identity = Binder.clearCallingIdentity();
+ try {
+ String number;
+ number = getPhoneNumberFromSourceInternal(
+ subId,
+ SubscriptionManager.PHONE_NUMBER_SOURCE_CARRIER);
+ if (!TextUtils.isEmpty(number)) return number;
+
+ number = getPhoneNumberFromSourceInternal(
+ subId,
+ SubscriptionManager.PHONE_NUMBER_SOURCE_UICC);
+ if (!TextUtils.isEmpty(number)) return number;
+
+ number = getPhoneNumberFromSourceInternal(
+ subId,
+ SubscriptionManager.PHONE_NUMBER_SOURCE_IMS);
+ return TextUtils.emptyIfNull(number);
+ } finally {
+ Binder.restoreCallingIdentity(identity);
+ }
+ } else {
+ String numberFromCarrier = getPhoneNumber(subId,
+ SubscriptionManager.PHONE_NUMBER_SOURCE_CARRIER, callingPackage,
+ callingFeatureId);
+ if (!TextUtils.isEmpty(numberFromCarrier)) {
+ return numberFromCarrier;
+ }
+ String numberFromUicc = getPhoneNumber(
+ subId, SubscriptionManager.PHONE_NUMBER_SOURCE_UICC, callingPackage,
+ callingFeatureId);
+ if (!TextUtils.isEmpty(numberFromUicc)) {
+ return numberFromUicc;
+ }
+ String numberFromIms = getPhoneNumber(
+ subId, SubscriptionManager.PHONE_NUMBER_SOURCE_IMS, callingPackage,
+ callingFeatureId);
+ if (!TextUtils.isEmpty(numberFromIms)) {
+ return numberFromIms;
+ }
+ return "";
}
- String numberFromUicc = getPhoneNumber(
- subId, SubscriptionManager.PHONE_NUMBER_SOURCE_UICC, callingPackage,
- callingFeatureId);
- if (!TextUtils.isEmpty(numberFromUicc)) {
- return numberFromUicc;
- }
- String numberFromIms = getPhoneNumber(
- subId, SubscriptionManager.PHONE_NUMBER_SOURCE_IMS, callingPackage,
- callingFeatureId);
- if (!TextUtils.isEmpty(numberFromIms)) {
- return numberFromIms;
- }
- return "";
}
/**
diff --git a/tests/telephonytests/src/com/android/internal/telephony/subscription/SubscriptionDatabaseManagerTest.java b/tests/telephonytests/src/com/android/internal/telephony/subscription/SubscriptionDatabaseManagerTest.java
index 7339e42..62b9def 100644
--- a/tests/telephonytests/src/com/android/internal/telephony/subscription/SubscriptionDatabaseManagerTest.java
+++ b/tests/telephonytests/src/com/android/internal/telephony/subscription/SubscriptionDatabaseManagerTest.java
@@ -76,8 +76,8 @@
static final String FAKE_DEFAULT_CARD_NAME = "CARD %d";
static final String FAKE_ICCID1 = "123456";
static final String FAKE_ICCID2 = "456789";
- static final String FAKE_PHONE_NUMBER1 = "6502530000";
- static final String FAKE_PHONE_NUMBER2 = "4089961010";
+ static final String FAKE_PHONE_NUMBER1 = "9995551234";
+ static final String FAKE_PHONE_NUMBER2 = "9998887777";
static final String FAKE_CARRIER_NAME1 = "A-Mobile";
static final String FAKE_CARRIER_NAME2 = "B-Mobile";
static final int FAKE_COLOR1 = 1;
diff --git a/tests/telephonytests/src/com/android/internal/telephony/subscription/SubscriptionManagerServiceTest.java b/tests/telephonytests/src/com/android/internal/telephony/subscription/SubscriptionManagerServiceTest.java
index 9fee311..e6877ee 100644
--- a/tests/telephonytests/src/com/android/internal/telephony/subscription/SubscriptionManagerServiceTest.java
+++ b/tests/telephonytests/src/com/android/internal/telephony/subscription/SubscriptionManagerServiceTest.java
@@ -243,7 +243,6 @@
doReturn(false).when(mFlags).enforceTelephonyFeatureMappingForPublicApis();
doReturn(true).when(mPackageManager).hasSystemFeature(
eq(PackageManager.FEATURE_TELEPHONY_SUBSCRIPTION));
-
logd("SubscriptionManagerServiceTest -Setup!");
}
@@ -1788,6 +1787,45 @@
}
@Test
+ public void testGetPhoneNumberSourcePriority() throws Exception {
+ mContextFixture.addCallingOrSelfPermission(Manifest.permission.READ_PHONE_NUMBERS);
+
+ String phoneNumberFromCarrier = "8675309";
+ String phoneNumberFromUicc = "1112223333";
+ String phoneNumberFromIms = "5553466";
+ String phoneNumberFromPhoneObject = "8001234567";
+
+ doReturn(phoneNumberFromPhoneObject).when(mPhone).getLine1Number();
+
+ SubscriptionInfoInternal multiNumberSubInfo =
+ new SubscriptionInfoInternal.Builder(FAKE_SUBSCRIPTION_INFO1)
+ .setNumberFromCarrier(phoneNumberFromCarrier)
+ .setNumber(phoneNumberFromUicc)
+ .setNumberFromIms(phoneNumberFromIms)
+ .build();
+ int subId = insertSubscription(multiNumberSubInfo);
+
+ assertThat(mSubscriptionManagerServiceUT.getPhoneNumberFromFirstAvailableSource(
+ subId, CALLING_PACKAGE, CALLING_FEATURE)).isEqualTo(phoneNumberFromCarrier);
+
+ multiNumberSubInfo =
+ new SubscriptionInfoInternal.Builder(multiNumberSubInfo)
+ .setNumberFromCarrier("")
+ .setNumber(phoneNumberFromUicc)
+ .setNumberFromIms(phoneNumberFromIms)
+ .build();
+ subId = insertSubscription(multiNumberSubInfo);
+
+ assertThat(mSubscriptionManagerServiceUT.getPhoneNumberFromFirstAvailableSource(
+ subId, CALLING_PACKAGE, CALLING_FEATURE)).isEqualTo(phoneNumberFromPhoneObject);
+
+ doReturn("").when(mPhone).getLine1Number();
+
+ assertThat(mSubscriptionManagerServiceUT.getPhoneNumberFromFirstAvailableSource(
+ subId, CALLING_PACKAGE, CALLING_FEATURE)).isEqualTo(phoneNumberFromIms);
+ }
+
+ @Test
public void testSetUiccApplicationsEnabled() {
insertSubscription(FAKE_SUBSCRIPTION_INFO1);
@@ -2483,6 +2521,39 @@
}
@Test
+ public void testGetPhoneNumberFromDefaultSubscription() {
+ doReturn(true).when(mFlags).saferGetPhoneNumber();
+
+ mContextFixture.addCallingOrSelfPermission(Manifest.permission.READ_PRIVILEGED_PHONE_STATE);
+ mContextFixture.addCallingOrSelfPermission(Manifest.permission.MODIFY_PHONE_STATE);
+ int subId = insertSubscription(FAKE_SUBSCRIPTION_INFO1);
+
+ mSubscriptionManagerServiceUT.setDefaultVoiceSubId(subId);
+
+ assertThat(
+ mSubscriptionManagerServiceUT.getPhoneNumberFromFirstAvailableSource(
+ subId, CALLING_PACKAGE, CALLING_FEATURE)).isEqualTo(FAKE_PHONE_NUMBER1);
+ assertThat(
+ mSubscriptionManagerServiceUT.getPhoneNumber(
+ SubscriptionManager.DEFAULT_SUBSCRIPTION_ID,
+ SubscriptionManager.PHONE_NUMBER_SOURCE_UICC,
+ CALLING_PACKAGE,
+ CALLING_FEATURE)).isEqualTo(FAKE_PHONE_NUMBER1);
+ assertThat(
+ mSubscriptionManagerServiceUT.getPhoneNumber(
+ SubscriptionManager.DEFAULT_SUBSCRIPTION_ID,
+ SubscriptionManager.PHONE_NUMBER_SOURCE_CARRIER,
+ CALLING_PACKAGE,
+ CALLING_FEATURE)).isEqualTo(FAKE_PHONE_NUMBER1);
+ assertThat(
+ mSubscriptionManagerServiceUT.getPhoneNumber(
+ SubscriptionManager.DEFAULT_SUBSCRIPTION_ID,
+ SubscriptionManager.PHONE_NUMBER_SOURCE_IMS,
+ CALLING_PACKAGE,
+ CALLING_FEATURE)).isEqualTo(FAKE_PHONE_NUMBER1);
+ }
+
+ @Test
public void testEsimActivation() {
mContextFixture.addCallingOrSelfPermission(Manifest.permission.READ_PRIVILEGED_PHONE_STATE);
mContextFixture.addCallingOrSelfPermission(Manifest.permission.MODIFY_PHONE_STATE);