Merge "Fix location settings bug on non-GPS devices" into pi-dev

am: b2637dac49

Change-Id: I538247bf9b4ccb735f5e84b7989683ea53344849

3 files changed, 177 insertions, 64 deletions

diff --git a/location/java/android/location/LocationManager.java b/location/java/android/location/LocationManager.java
index 2dd8c36f1165..c16876b966bc 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<String> allProvidersList = getAllProviders();
+        // Update all providers on device plus gps and network provider when disabling location.
+        Set<String> 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 f215c1220610..b37071bf24cf 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);
-            }
-            return false;
-        }
-
-        // skip prefix
-        value = value.substring(1);
-
-        Setting settingValue = getSecureSetting(
-                    Settings.Secure.LOCATION_PROVIDERS_ALLOWED, owningUserId);
-        if (settingValue == null) {
+        Setting oldSetting = getSecureSetting(
+                Settings.Secure.LOCATION_PROVIDERS_ALLOWED, owningUserId);
+        if (oldSetting == 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 oldProviders = oldSetting.getValue();
+        List<String> oldProvidersList = TextUtils.isEmpty(oldProviders)
+                ? new ArrayList<>() : new ArrayList<>(Arrays.asList(oldProviders.split(",")));
+        Set<String> newProvidersSet = new ArraySet<>();
+        newProvidersSet.addAll(oldProvidersList);
+
+        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 e2a8fba2f1c7..b6f51bc4d781 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));
+    }
 }