From 83e03f55597a69bf1e1f2df2cd24e83e84d21914 Mon Sep 17 00:00:00 2001 From: Maggie Date: Fri, 16 Mar 2018 12:22:20 -0700 Subject: [PATCH] Fix location settings bug on non-GPS devices The old location_mode API hardcoded gps and network location provider when it enables/disables location, without checking whether the providers exist on device. It causes bugs when used together with the new LocationManager.setLocationEnabled() APIs. This fix modified LocationManager.setLocationEnabled() API when user tries to disable location on device. Besides turning off the providers from LocationManager.getAllProviders(), it also turns off GPS and network provider explicitly. To reduce times of binding to the service and chance of race condition, we also modified SettingsProvider.updateLocationProvidersAllowedLocked() to accept a string param with multiple location providers to be enabled or disalbed at the same time. Bug: 73261572 Test: Manual on chromebook Change-Id: I2e59e0d4cf395b98cd481af5d7f3c762274d7826 --- .../android/location/LocationManager.java | 34 +++++- .../providers/settings/SettingsProvider.java | 90 +++++--------- .../settings/SettingsProviderTest.java | 113 +++++++++++++++++- 3 files changed, 175 insertions(+), 62 deletions(-) diff --git a/location/java/android/location/LocationManager.java b/location/java/android/location/LocationManager.java index 2dd8c36f11653..c16876b966bcc 100644 --- a/location/java/android/location/LocationManager.java +++ b/location/java/android/location/LocationManager.java @@ -42,12 +42,14 @@ import android.os.RemoteException; import android.os.UserHandle; import android.provider.Settings; import android.text.TextUtils; +import android.util.ArraySet; import android.util.Log; import com.android.internal.location.ProviderProperties; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.List; +import java.util.Set; /** * This class provides access to the system location services. These @@ -1252,12 +1254,40 @@ public class LocationManager { @SystemApi @RequiresPermission(WRITE_SECURE_SETTINGS) public void setLocationEnabledForUser(boolean enabled, UserHandle userHandle) { - for (String provider : getAllProviders()) { + final List allProvidersList = getAllProviders(); + // Update all providers on device plus gps and network provider when disabling location. + Set allProvidersSet = new ArraySet<>(allProvidersList.size() + 2); + allProvidersSet.addAll(allProvidersList); + // When disabling location, disable gps and network provider that could have been enabled by + // location mode api. + if (enabled == false) { + allProvidersSet.add(GPS_PROVIDER); + allProvidersSet.add(NETWORK_PROVIDER); + } + if (allProvidersSet.isEmpty()) { + return; + } + // to ensure thread safety, we write the provider name with a '+' or '-' + // and let the SettingsProvider handle it rather than reading and modifying + // the list of enabled providers. + final String prefix = enabled ? "+" : "-"; + StringBuilder locationProvidersAllowed = new StringBuilder(); + for (String provider : allProvidersSet) { + checkProvider(provider); if (provider.equals(PASSIVE_PROVIDER)) { continue; } - setProviderEnabledForUser(provider, enabled, userHandle); + locationProvidersAllowed.append(prefix); + locationProvidersAllowed.append(provider); + locationProvidersAllowed.append(","); } + // Remove the trailing comma + locationProvidersAllowed.setLength(locationProvidersAllowed.length() - 1); + Settings.Secure.putStringForUser( + mContext.getContentResolver(), + Settings.Secure.LOCATION_PROVIDERS_ALLOWED, + locationProvidersAllowed.toString(), + userHandle.getIdentifier()); } /** diff --git a/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java b/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java index 5102cbdf4c556..535bdbc818a22 100644 --- a/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java +++ b/packages/SettingsProvider/src/com/android/providers/settings/SettingsProvider.java @@ -1798,76 +1798,52 @@ public class SettingsProvider extends ContentProvider { if (TextUtils.isEmpty(value)) { return false; } - - final char prefix = value.charAt(0); - if (prefix != '+' && prefix != '-') { - if (forceNotify) { - final int key = makeKey(SETTINGS_TYPE_SECURE, owningUserId); - mSettingsRegistry.notifyForSettingsChange(key, - Settings.Secure.LOCATION_PROVIDERS_ALLOWED); - } + Setting oldSetting = getSecureSetting( + Settings.Secure.LOCATION_PROVIDERS_ALLOWED, owningUserId); + if (oldSetting == null) { return false; } + String oldProviders = oldSetting.getValue(); + List oldProvidersList = TextUtils.isEmpty(oldProviders) + ? new ArrayList<>() : new ArrayList<>(Arrays.asList(oldProviders.split(","))); + Set newProvidersSet = new ArraySet<>(); + newProvidersSet.addAll(oldProvidersList); - // skip prefix - value = value.substring(1); - - Setting settingValue = getSecureSetting( - Settings.Secure.LOCATION_PROVIDERS_ALLOWED, owningUserId); - if (settingValue == null) { - return false; - } - - String oldProviders = !settingValue.isNull() ? settingValue.getValue() : ""; - - int index = oldProviders.indexOf(value); - int end = index + value.length(); - - // check for commas to avoid matching on partial string - if (index > 0 && oldProviders.charAt(index - 1) != ',') { - index = -1; - } - - // check for commas to avoid matching on partial string - if (end < oldProviders.length() && oldProviders.charAt(end) != ',') { - index = -1; - } - - String newProviders; - - if (prefix == '+' && index < 0) { - // append the provider to the list if not present - if (oldProviders.length() == 0) { - newProviders = value; - } else { - newProviders = oldProviders + ',' + value; + String[] providerUpdates = value.split(","); + boolean inputError = false; + for (String provider : providerUpdates) { + // do not update location_providers_allowed when input is invalid + if (TextUtils.isEmpty(provider)) { + inputError = true; + break; } - } else if (prefix == '-' && index >= 0) { - // remove the provider from the list if present - // remove leading or trailing comma - if (index > 0) { - index--; - } else if (end < oldProviders.length()) { - end++; + final char prefix = provider.charAt(0); + // do not update location_providers_allowed when input is invalid + if (prefix != '+' && prefix != '-') { + inputError = true; + break; } - - newProviders = oldProviders.substring(0, index); - if (end < oldProviders.length()) { - newProviders += oldProviders.substring(end); + // skip prefix + provider = provider.substring(1); + if (prefix == '+') { + newProvidersSet.add(provider); + } else if (prefix == '-') { + newProvidersSet.remove(provider); } - } else { + } + String newProviders = TextUtils.join(",", newProvidersSet.toArray()); + if (inputError == true || newProviders.equals(oldProviders)) { // nothing changed, so no need to update the database if (forceNotify) { - final int key = makeKey(SETTINGS_TYPE_SECURE, owningUserId); - mSettingsRegistry.notifyForSettingsChange(key, + mSettingsRegistry.notifyForSettingsChange( + makeKey(SETTINGS_TYPE_SECURE, owningUserId), Settings.Secure.LOCATION_PROVIDERS_ALLOWED); } return false; } - return mSettingsRegistry.insertSettingLocked(SETTINGS_TYPE_SECURE, - owningUserId, Settings.Secure.LOCATION_PROVIDERS_ALLOWED, newProviders, - tag, makeDefault, getCallingPackage(), forceNotify, CRITICAL_SECURE_SETTINGS); + owningUserId, Settings.Secure.LOCATION_PROVIDERS_ALLOWED, newProviders, tag, + makeDefault, getCallingPackage(), forceNotify, CRITICAL_SECURE_SETTINGS); } private static void warnOrThrowForUndesiredSecureSettingsMutationForTargetSdk( diff --git a/packages/SettingsProvider/test/src/com/android/providers/settings/SettingsProviderTest.java b/packages/SettingsProvider/test/src/com/android/providers/settings/SettingsProviderTest.java index e2a8fba2f1c7c..b6f51bc4d781f 100644 --- a/packages/SettingsProvider/test/src/com/android/providers/settings/SettingsProviderTest.java +++ b/packages/SettingsProvider/test/src/com/android/providers/settings/SettingsProviderTest.java @@ -17,8 +17,8 @@ package com.android.providers.settings; import static junit.framework.Assert.assertEquals; -import static junit.framework.Assert.assertSame; import static junit.framework.Assert.assertNull; +import static junit.framework.Assert.assertSame; import static junit.framework.Assert.fail; import android.content.ContentResolver; @@ -32,9 +32,8 @@ import android.os.SystemClock; import android.os.UserHandle; import android.provider.Settings; import android.util.Log; -import org.junit.Test; - import java.util.concurrent.atomic.AtomicBoolean; +import org.junit.Test; /** * Tests for the SettingContentProvider. @@ -688,4 +687,112 @@ public class SettingsProviderTest extends BaseSettingsProviderTest { cursor.close(); } } + + @Test + public void testUpdateLocationProvidersAllowedLocked_enableProviders() throws Exception { + setSettingViaFrontEndApiAndAssertSuccessfulChange( + SETTING_TYPE_SECURE, + Settings.Secure.LOCATION_MODE, + String.valueOf(Settings.Secure.LOCATION_MODE_OFF), + UserHandle.USER_SYSTEM); + + // Enable one provider + updateStringViaProviderApiSetting( + SETTING_TYPE_SECURE, Settings.Secure.LOCATION_PROVIDERS_ALLOWED, "+gps"); + + assertEquals( + "Wrong location providers", + "gps", + queryStringViaProviderApi( + SETTING_TYPE_SECURE, Settings.Secure.LOCATION_PROVIDERS_ALLOWED)); + + // Enable a list of providers, including the one that is already enabled + updateStringViaProviderApiSetting( + SETTING_TYPE_SECURE, + Settings.Secure.LOCATION_PROVIDERS_ALLOWED, + "+gps,+network,+network"); + + assertEquals( + "Wrong location providers", + "gps,network", + queryStringViaProviderApi( + SETTING_TYPE_SECURE, Settings.Secure.LOCATION_PROVIDERS_ALLOWED)); + } + + @Test + public void testUpdateLocationProvidersAllowedLocked_disableProviders() throws Exception { + setSettingViaFrontEndApiAndAssertSuccessfulChange( + SETTING_TYPE_SECURE, + Settings.Secure.LOCATION_MODE, + String.valueOf(Settings.Secure.LOCATION_MODE_HIGH_ACCURACY), + UserHandle.USER_SYSTEM); + + // Disable providers that were enabled + updateStringViaProviderApiSetting( + SETTING_TYPE_SECURE, + Settings.Secure.LOCATION_PROVIDERS_ALLOWED, + "-gps,-network"); + + assertEquals( + "Wrong location providers", + "", + queryStringViaProviderApi( + SETTING_TYPE_SECURE, Settings.Secure.LOCATION_PROVIDERS_ALLOWED)); + + // Disable a provider that was not enabled + updateStringViaProviderApiSetting( + SETTING_TYPE_SECURE, + Settings.Secure.LOCATION_PROVIDERS_ALLOWED, + "-test"); + + assertEquals( + "Wrong location providers", + "", + queryStringViaProviderApi( + SETTING_TYPE_SECURE, Settings.Secure.LOCATION_PROVIDERS_ALLOWED)); + } + + @Test + public void testUpdateLocationProvidersAllowedLocked_enableAndDisable() throws Exception { + setSettingViaFrontEndApiAndAssertSuccessfulChange( + SETTING_TYPE_SECURE, + Settings.Secure.LOCATION_MODE, + String.valueOf(Settings.Secure.LOCATION_MODE_OFF), + UserHandle.USER_SYSTEM); + + updateStringViaProviderApiSetting( + SETTING_TYPE_SECURE, + Settings.Secure.LOCATION_PROVIDERS_ALLOWED, + "+gps,+network,+test"); + updateStringViaProviderApiSetting( + SETTING_TYPE_SECURE, Settings.Secure.LOCATION_PROVIDERS_ALLOWED, "-test"); + + assertEquals( + "Wrong location providers", + "gps,network", + queryStringViaProviderApi( + SETTING_TYPE_SECURE, Settings.Secure.LOCATION_PROVIDERS_ALLOWED)); + } + + @Test + public void testUpdateLocationProvidersAllowedLocked_invalidInput() throws Exception { + setSettingViaFrontEndApiAndAssertSuccessfulChange( + SETTING_TYPE_SECURE, + Settings.Secure.LOCATION_MODE, + String.valueOf(Settings.Secure.LOCATION_MODE_OFF), + UserHandle.USER_SYSTEM); + + // update providers with a invalid string + updateStringViaProviderApiSetting( + SETTING_TYPE_SECURE, + Settings.Secure.LOCATION_PROVIDERS_ALLOWED, + "+gps, invalid-string"); + + // Verifies providers list does not change + assertEquals( + "Wrong location providers", + "", + queryStringViaProviderApi( + SETTING_TYPE_SECURE, Settings.Secure.LOCATION_PROVIDERS_ALLOWED)); + } }