Update default value of probe url to be a constant

Default value for probing url should be a constant in
NetworkStack but not an overlayable config due to some
technical limitations. OEMs can mistakenly override configs
that were not designed to be overridden.

Bug: 152730542
Test: atest NetworkStackTests NetworkStackNextTests
Change-Id: I1846958e7c4e8b64ae287718c63e705bb232632a
Merged-In: I1846958e7c4e8b64ae287718c63e705bb232632a

2 files changed, 41 insertions, 20 deletions

diff --git a/src/android/net/util/NetworkStackUtils.java b/src/android/net/util/NetworkStackUtils.java
index be4c2e4..3f63266 100755
--- a/src/android/net/util/NetworkStackUtils.java
+++ b/src/android/net/util/NetworkStackUtils.java
@@ -155,16 +155,30 @@ public class NetworkStackUtils {
     public static final int CAPTIVE_PORTAL_MODE_AVOID = 2;
 
     /**
+     * DNS probe timeout for network validation. Enough for 3 DNS queries 5 seconds apart.
+     */
+    public static final int DEFAULT_CAPTIVE_PORTAL_DNS_PROBE_TIMEOUT = 12500;
+
+    /**
+     * List of fallback probe specs to use for detecting captive portals. This is an alternative to
+     * fallback URLs that provides more flexibility on detection rules. Empty, so unused by default.
+     */
+    public static final String[] DEFAULT_CAPTIVE_PORTAL_FALLBACK_PROBE_SPECS =
+            new String[] {};
+
+    /**
      * The default list of HTTP URLs to use for detecting captive portals.
      */
     public static final String[] DEFAULT_CAPTIVE_PORTAL_HTTP_URLS =
             new String [] {"http://connectivitycheck.gstatic.com/generate_204"};
+
     /**
      * The default list of HTTPS URLs for network validation, to use for confirming internet
      * connectivity.
      */
     public static final String[] DEFAULT_CAPTIVE_PORTAL_HTTPS_URLS =
             new String [] {"https://www.google.com/generate_204"};
+
     /**
      * @deprecated Considering boolean experiment flag is likely to cause misconfiguration
      *             particularly when NetworkStack module rolls back to previous version. It's
diff --git a/src/com/android/server/connectivity/NetworkMonitor.java b/src/com/android/server/connectivity/NetworkMonitor.java
index 052fb4f..6d9cab4 100755
--- a/src/com/android/server/connectivity/NetworkMonitor.java
+++ b/src/com/android/server/connectivity/NetworkMonitor.java
@@ -67,6 +67,8 @@ import static android.net.util.NetworkStackUtils.CAPTIVE_PORTAL_OTHER_HTTPS_URLS
 import static android.net.util.NetworkStackUtils.CAPTIVE_PORTAL_OTHER_HTTP_URLS;
 import static android.net.util.NetworkStackUtils.CAPTIVE_PORTAL_USER_AGENT;
 import static android.net.util.NetworkStackUtils.CAPTIVE_PORTAL_USE_HTTPS;
+import static android.net.util.NetworkStackUtils.DEFAULT_CAPTIVE_PORTAL_DNS_PROBE_TIMEOUT;
+import static android.net.util.NetworkStackUtils.DEFAULT_CAPTIVE_PORTAL_FALLBACK_PROBE_SPECS;
 import static android.net.util.NetworkStackUtils.DEFAULT_CAPTIVE_PORTAL_HTTPS_URLS;
 import static android.net.util.NetworkStackUtils.DEFAULT_CAPTIVE_PORTAL_HTTP_URLS;
 import static android.net.util.NetworkStackUtils.DISMISS_PORTAL_IN_VALIDATED_NETWORK;
@@ -1801,8 +1803,9 @@ public class NetworkMonitor extends StateMachine {
         final String testUrl = getTestUrl(TEST_CAPTIVE_PORTAL_HTTPS_URL);
         if (isValidTestUrl(testUrl)) return testUrl;
         final Context targetContext = getCustomizedContextOrDefault();
-        return getSettingFromResource(targetContext, R.string.config_captive_portal_https_url,
-                R.string.default_captive_portal_https_url, CAPTIVE_PORTAL_HTTPS_URL);
+        return getSettingFromResource(targetContext,
+                R.string.config_captive_portal_https_url, CAPTIVE_PORTAL_HTTPS_URL,
+                targetContext.getResources().getString(R.string.default_captive_portal_https_url));
     }
 
     private static boolean isValidTestUrl(@Nullable String url) {
@@ -1819,14 +1822,13 @@ public class NetworkMonitor extends StateMachine {
 
     private int getDnsProbeTimeout() {
         return getIntSetting(mContext, R.integer.config_captive_portal_dns_probe_timeout,
-                CONFIG_CAPTIVE_PORTAL_DNS_PROBE_TIMEOUT,
-                R.integer.default_captive_portal_dns_probe_timeout);
+                CONFIG_CAPTIVE_PORTAL_DNS_PROBE_TIMEOUT, DEFAULT_CAPTIVE_PORTAL_DNS_PROBE_TIMEOUT);
     }
 
     /**
      * Gets an integer setting from resources or device config
      *
-     * configResource is used if set, followed by device config if set, followed by defaultResource.
+     * configResource is used if set, followed by device config if set, followed by defaultValue.
      * If none of these are set then an exception is thrown.
      *
      * TODO: move to a common location such as a ConfigUtils class.
@@ -1834,13 +1836,13 @@ public class NetworkMonitor extends StateMachine {
      */
     @VisibleForTesting
     int getIntSetting(@NonNull final Context context, @StringRes int configResource,
-            @NonNull String symbol, @StringRes int defaultResource) {
+            @NonNull String symbol, int defaultValue) {
         final Resources res = context.getResources();
         try {
             return res.getInteger(configResource);
         } catch (Resources.NotFoundException e) {
             return mDependencies.getDeviceConfigPropertyInt(NAMESPACE_CONNECTIVITY,
-                    symbol, res.getInteger(defaultResource));
+                    symbol, defaultValue);
         }
     }
 
@@ -1894,8 +1896,9 @@ public class NetworkMonitor extends StateMachine {
         final String testUrl = getTestUrl(TEST_CAPTIVE_PORTAL_HTTP_URL);
         if (isValidTestUrl(testUrl)) return testUrl;
         final Context targetContext = getCustomizedContextOrDefault();
-        return getSettingFromResource(targetContext, R.string.config_captive_portal_http_url,
-                R.string.default_captive_portal_http_url, CAPTIVE_PORTAL_HTTP_URL);
+        return getSettingFromResource(targetContext,
+                R.string.config_captive_portal_http_url, CAPTIVE_PORTAL_HTTP_URL,
+                targetContext.getResources().getString(R.string.default_captive_portal_http_url));
     }
 
     private int getConsecutiveDnsTimeoutThreshold() {
@@ -1937,7 +1940,8 @@ public class NetworkMonitor extends StateMachine {
                 combineCaptivePortalUrls(firstUrl, CAPTIVE_PORTAL_OTHER_FALLBACK_URLS);
             return getProbeUrlArrayConfig(settingProviderUrls,
                     R.array.config_captive_portal_fallback_urls,
-                    R.array.default_captive_portal_fallback_urls, this::makeURL);
+                    R.array.default_captive_portal_fallback_urls,
+                    this::makeURL);
         } catch (Exception e) {
             // Don't let a misconfiguration bootloop the system.
             Log.e(TAG, "Error parsing configured fallback URLs", e);
@@ -1957,7 +1961,7 @@ public class NetworkMonitor extends StateMachine {
 
             return getProbeUrlArrayConfig(providerValue,
                     R.array.config_captive_portal_fallback_probe_specs,
-                    R.array.default_captive_portal_fallback_probe_specs,
+                    DEFAULT_CAPTIVE_PORTAL_FALLBACK_PROBE_SPECS,
                     CaptivePortalProbeSpec::parseSpecOrNull);
         } catch (Exception e) {
             // Don't let a misconfiguration bootloop the system.
@@ -1971,6 +1975,8 @@ public class NetworkMonitor extends StateMachine {
         try {
             final URL[] settingProviderUrls =
                 combineCaptivePortalUrls(firstUrl, CAPTIVE_PORTAL_OTHER_HTTPS_URLS);
+            // firstUrl will at least be default configuration, so default value in
+            // getProbeUrlArrayConfig is actually never used.
             return getProbeUrlArrayConfig(settingProviderUrls,
                     R.array.config_captive_portal_https_urls,
                     DEFAULT_CAPTIVE_PORTAL_HTTPS_URLS, this::makeURL);
@@ -1987,6 +1993,8 @@ public class NetworkMonitor extends StateMachine {
         try {
             final URL[] settingProviderUrls =
                     combineCaptivePortalUrls(firstUrl, CAPTIVE_PORTAL_OTHER_HTTP_URLS);
+            // firstUrl will at least be default configuration, so default value in
+            // getProbeUrlArrayConfig is actually never used.
             return getProbeUrlArrayConfig(settingProviderUrls,
                     R.array.config_captive_portal_http_urls,
                     DEFAULT_CAPTIVE_PORTAL_HTTP_URLS, this::makeURL);
@@ -1998,11 +2006,11 @@ public class NetworkMonitor extends StateMachine {
         }
     }
 
-    private URL[] combineCaptivePortalUrls(final String firstUrl, final String name) {
+    private URL[] combineCaptivePortalUrls(final String firstUrl, final String propertyName) {
         if (TextUtils.isEmpty(firstUrl)) return new URL[0];
 
         final String otherUrls = mDependencies.getDeviceConfigProperty(
-                NAMESPACE_CONNECTIVITY, name, "");
+                NAMESPACE_CONNECTIVITY, propertyName, "");
         // otherUrls may be empty, but .split() ignores trailing empty strings
         final String separator = ",";
         final String[] urls = (firstUrl + separator + otherUrls).split(separator);
@@ -2012,27 +2020,26 @@ public class NetworkMonitor extends StateMachine {
     /**
      * Read a setting from a resource or the settings provider.
      *
-     * <p>The configuration resource is prioritized, then the provider value, then the default
-     * resource value.
+     * <p>The configuration resource is prioritized, then the provider value.
      * @param context The context
      * @param configResource The resource id for the configuration parameter
-     * @param defaultResource The resource id for the default value
      * @param symbol The symbol in the settings provider
+     * @param defaultValue The default value
      * @return The best available value
      */
-    @NonNull
+    @Nullable
     private String getSettingFromResource(@NonNull final Context context,
-            @StringRes int configResource, @StringRes int defaultResource,
-            @NonNull String symbol) {
+            @StringRes int configResource, @NonNull String symbol, @NonNull String defaultValue) {
         final Resources res = context.getResources();
         String setting = res.getString(configResource);
 
         if (!TextUtils.isEmpty(setting)) return setting;
 
         setting = mDependencies.getSetting(context, symbol, null);
+
         if (!TextUtils.isEmpty(setting)) return setting;
 
-        return res.getString(defaultResource);
+        return defaultValue;
     }
 
     /**