Preserve custom DNS addresses when switching to network DNS

app/src/main/java/eu/faircode/netguard/ActivitySettings.java

@@ -803,6 +803,9 @@ else if ("vpn4".equals(name)) {
                     getString(R.string.setting_dns, prefs.getString(name, "-")));
             ServiceSinkhole.reload("changed " + name, this, false);
 
+        } else if ("custom_dns_enabled".equals(name)) {
+            ServiceSinkhole.reload("changed " + name, this, false);
+
         } else if ("doh_enabled".equals(name)) {
             // Reset DoH client when toggled
             net.kollnig.missioncontrol.dns.DnsOverHttpsClient.resetInstance();

app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java

@@ -1584,8 +1584,11 @@ public static List<InetAddress> getDns(Context context) {
         // Get custom DNS servers
         SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context);
         boolean ip6 = prefs.getBoolean("ip6", true);
-        String vpnDns1 = prefs.getString("dns", null);
-        String vpnDns2 = prefs.getString("dns2", null);
+        // Preserve saved addresses when temporarily using the network's DNS.
+        // Default on so upgrades and older backups retain their DNS behaviour.
+        boolean customDns = prefs.getBoolean("custom_dns_enabled", true);
+        String vpnDns1 = customDns ? prefs.getString("dns", null) : null;
+        String vpnDns2 = customDns ? prefs.getString("dns2", null) : null;
         Log.i(TAG, "DNS system=" + TextUtils.join(",", sysDns) + " config=" + vpnDns1 + "," + vpnDns2);
 
         // First pass: IPv4 resolvers only. This reproduces the historical

app/src/main/java/net/kollnig/missioncontrol/LocalNetworkAccess.java

@@ -94,7 +94,7 @@ private LocalNetworkAccess() {
 
     /** Preferences that can point TrackerControl at the local network. */
     private static final String[] SETTINGS = {
-            "dns", "dns2",                      // custom VPN DNS servers
+            "dns", "dns2", "custom_dns_enabled", // custom VPN DNS servers
             "doh_enabled", "doh_endpoint",      // Secure DNS
             "tcp_mss_clamp",                    // tethering compatibility mode
             "wg_enabled", "wg_config",          // WireGuard remote egress
@@ -220,8 +220,9 @@ private static boolean isConfigured(Context context) {
     }
 
     static boolean isConfigured(SharedPreferences prefs) {
-        if (isLocalAddress(prefs.getString("dns", null)) ||
-                isLocalAddress(prefs.getString("dns2", null)))
+        if (prefs.getBoolean("custom_dns_enabled", true) &&
+                (isLocalAddress(prefs.getString("dns", null)) ||
+                        isLocalAddress(prefs.getString("dns2", null))))
             return true;
 
         if (prefs.getBoolean("doh_enabled", false) &&

app/src/main/res/values/strings.xml

@@ -163,6 +163,8 @@
     <string name="setting_vpn4">VPN IPv4: %s</string>
     <string name="setting_vpn6">VPN IPv6: %s</string>
     <string name="setting_dns">VPN DNS: %s</string>
+    <string name="setting_custom_dns_enabled">Use custom DNS</string>
+    <string name="summary_custom_dns_enabled">Turn off to use your network’s DNS without deleting the saved addresses. Secure DNS and remote VPN DNS take precedence.</string>
     <string name="setting_validate">Validate at: %s</string>
     <string name="setting_ttl">Minimum DNS TTL: %s s</string>
     <string name="setting_socks5_enabled">Use SOCKS5 proxy</string>

app/src/main/res/xml/preferences.xml

@@ -145,10 +145,17 @@
                 android:hint="fd00:1:fd00:1:fd00:1:fd00:1"
                 android:inputType="text"
                 android:key="vpn6" />
+            <eu.faircode.netguard.SwitchPreference
+                android:defaultValue="true"
+                android:key="custom_dns_enabled"
+                android:summary="@string/summary_custom_dns_enabled"
+                android:title="@string/setting_custom_dns_enabled" />
             <EditTextPreference
+                android:dependency="custom_dns_enabled"
                 android:inputType="text"
                 android:key="dns" />
             <EditTextPreference
+                android:dependency="custom_dns_enabled"
                 android:inputType="text"
                 android:key="dns2" />
             <EditTextPreference

app/src/test/java/eu/faircode/netguard/CustomDnsTest.java

@@ -0,0 +1,110 @@
+package eu.faircode.netguard;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.robolectric.Shadows.shadowOf;
+
+import android.content.Context;
+import android.content.SharedPreferences;
+import android.net.ConnectivityManager;
+import android.net.LinkProperties;
+import android.net.Network;
+
+import androidx.preference.EditTextPreference;
+import androidx.preference.PreferenceManager;
+import androidx.preference.PreferenceScreen;
+
+import net.kollnig.missioncontrol.R;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.RobolectricTestRunner;
+import org.robolectric.RuntimeEnvironment;
+import org.robolectric.util.ReflectionHelpers;
+
+import java.net.InetAddress;
+import java.util.Arrays;
+import java.util.Collections;
+
+@RunWith(RobolectricTestRunner.class)
+public class CustomDnsTest {
+    private Context context;
+    private SharedPreferences prefs;
+
+    @Before
+    public void setUp() throws Exception {
+        context = RuntimeEnvironment.getApplication();
+        prefs = PreferenceManager.getDefaultSharedPreferences(context);
+        prefs.edit().clear().putString("dns", "9.9.9.9")
+                .putString("dns2", "149.112.112.112").commit();
+        setNetworkDns("192.168.1.1");
+    }
+
+    @Test
+    public void olderSettingsKeepCustomDnsAndToggleRestoresSavedPair() throws Exception {
+        assertEquals(Arrays.asList(InetAddress.getByName("9.9.9.9"),
+                InetAddress.getByName("149.112.112.112")), ServiceSinkhole.getDns(context));
+
+        prefs.edit().putBoolean("custom_dns_enabled", false).commit();
+        assertEquals(Collections.singletonList(InetAddress.getByName("192.168.1.1")),
+                ServiceSinkhole.getDns(context));
+        assertEquals("9.9.9.9", prefs.getString("dns", null));
+        assertEquals("149.112.112.112", prefs.getString("dns2", null));
+
+        prefs.edit().putBoolean("custom_dns_enabled", true).commit();
+        assertEquals(Arrays.asList(InetAddress.getByName("9.9.9.9"),
+                InetAddress.getByName("149.112.112.112")), ServiceSinkhole.getDns(context));
+    }
+
+    @Test
+    public void disabledCustomDnsUsesIpv6OnlyNetworkResolver() throws Exception {
+        setNetworkDns("fd00::53");
+        prefs.edit().putBoolean("custom_dns_enabled", false).commit();
+        assertEquals(Collections.singletonList(InetAddress.getByName("fd00::53")),
+                ServiceSinkhole.getDns(context));
+    }
+
+    @Test
+    public void enabledWithoutSavedAddressesStillUsesNetworkDns() throws Exception {
+        prefs.edit().remove("dns").remove("dns2").commit();
+        assertEquals(Collections.singletonList(InetAddress.getByName("192.168.1.1")),
+                ServiceSinkhole.getDns(context));
+    }
+
+    @Test
+    public void preferenceDisablesFieldsWithoutErasingTheirValues() {
+        PreferenceManager manager = new PreferenceManager(context);
+        PreferenceScreen screen = manager.inflateFromResource(context, R.xml.preferences, null);
+        manager.setPreferences(screen);
+        screen.onAttached();
+        try {
+            SwitchPreference toggle = screen.findPreference("custom_dns_enabled");
+            EditTextPreference primary = screen.findPreference("dns");
+            EditTextPreference secondary = screen.findPreference("dns2");
+            assertTrue(toggle.isChecked());
+            toggle.setChecked(false);
+            assertFalse(primary.isEnabled());
+            assertFalse(secondary.isEnabled());
+            assertEquals("9.9.9.9", primary.getText());
+            assertEquals("149.112.112.112", secondary.getText());
+            toggle.setChecked(true);
+            assertTrue(primary.isEnabled());
+            assertTrue(secondary.isEnabled());
+        } finally {
+            screen.onDetached();
+        }
+    }
+
+    private void setNetworkDns(String address) throws Exception {
+        ConnectivityManager manager = context.getSystemService(ConnectivityManager.class);
+        Network network = manager.getActiveNetwork();
+        assertNotNull(network);
+        LinkProperties properties = new LinkProperties();
+        ReflectionHelpers.callInstanceMethod(properties, "addDnsServer",
+                ReflectionHelpers.ClassParameter.from(InetAddress.class, InetAddress.getByName(address)));
+        shadowOf(manager).setLinkProperties(network, properties);
+    }
+}

app/src/test/java/net/kollnig/missioncontrol/LocalNetworkAccessTest.java

@@ -171,6 +171,17 @@ public void privateSecondaryDnsNeedsLocalNetworkAccess() {
         assertTrue(LocalNetworkAccess.isConfigured(prefs));
     }
 
+    @Test
+    public void disabledCustomDnsDoesNotRequestAccessForSavedLocalServers() {
+        prefs.edit().putString("dns", "192.168.1.10").putString("dns2", "fd00::53")
+                .putBoolean("custom_dns_enabled", false).commit();
+        assertFalse(LocalNetworkAccess.isConfigured(prefs));
+        assertTrue(LocalNetworkAccess.isRelevantSetting("custom_dns_enabled"));
+
+        prefs.edit().putBoolean("custom_dns_enabled", true).commit();
+        assertTrue(LocalNetworkAccess.isConfigured(prefs));
+    }
+
     @Test
     public void uniqueLocalIpv6DnsNeedsLocalNetworkAccess() {
         prefs.edit().putString("dns", "fd12:3456:789a::1").commit();

app/src/test/java/net/kollnig/missioncontrol/wg/WgConnectivityMonitorTest.kt

@@ -23,7 +23,8 @@ class WgConnectivityMonitorTest {
     }
 
     private fun awaitStopped(monitor: WgConnectivityMonitor) {
-        repeat(1_000) {
+        val deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5)
+        while (System.nanoTime() < deadline) {
             if (!monitor.isRunning()) return
             Thread.sleep(1)
         }
@@ -353,25 +354,28 @@ class WgConnectivityMonitorTest {
             clock = { 0L },
             beforeCallback = {
                 callbackReady.countDown()
-                assertTrue(releaseCallback.await(1, TimeUnit.SECONDS))
+                releaseCallback.await()
             }
         )
 
-        monitor.start()
-        assertTrue(callbackReady.await(1, TimeUnit.SECONDS))
-        Thread {
+        val stopper = Thread {
             monitor!!.stop()
             stopReturned.countDown()
-        }.start()
-        // stop invalidates the generation before waiting for the callback gate.
-        repeat(1_000) {
-            if (!monitor.isRunning()) return@repeat
-            Thread.yield()
         }
-        assertFalse(monitor.isRunning())
-        releaseCallback.countDown()
-        assertTrue(stopReturned.await(1, TimeUnit.SECONDS))
-        assertEquals(0, callbacks.get())
+        monitor.start()
+        try {
+            assertTrue(callbackReady.await(5, TimeUnit.SECONDS))
+            stopper.start()
+            // Wait for invalidation, not an arbitrary number of scheduler hints.
+            awaitStopped(monitor)
+            releaseCallback.countDown()
+            assertTrue(stopReturned.await(5, TimeUnit.SECONDS))
+            assertEquals(0, callbacks.get())
+        } finally {
+            releaseCallback.countDown()
+            monitor.stop()
+            stopper.join(5_000)
+        }
     }
 
     @Test
@@ -400,29 +404,31 @@ class WgConnectivityMonitorTest {
             clock = { now.get() },
             beforeCallback = {
                 prodReady.countDown()
-                assertTrue(releaseProd.await(1, TimeUnit.SECONDS))
+                releaseProd.await()
             }
         )
 
-        monitor.start()
-        assertTrue(prodReady.await(1, TimeUnit.SECONDS))
-        Thread {
+        val stopper = Thread {
             monitor!!.stop()
             stopReturned.countDown()
-        }.start()
-        // stop invalidates the generation first, then waits for the callback
-        // gate held by beforeCallback. It cannot return while that gate is
-        // unresolved.
-        repeat(1_000) {
-            if (!monitor.isRunning()) return@repeat
-            Thread.yield()
         }
-        assertFalse(monitor.isRunning())
-        assertFalse(stopReturned.await(100, TimeUnit.MILLISECONDS))
-        releaseProd.countDown()
-        assertTrue(stopReturned.await(1, TimeUnit.SECONDS))
-        awaitStopped(monitor)
-        assertEquals(0, prods.get())
+        monitor.start()
+        try {
+            assertTrue(prodReady.await(5, TimeUnit.SECONDS))
+            stopper.start()
+            // stop invalidates the generation first, then waits for the callback
+            // gate held by beforeCallback. It cannot return while that gate is
+            // unresolved.
+            awaitStopped(monitor)
+            assertFalse(stopReturned.await(100, TimeUnit.MILLISECONDS))
+            releaseProd.countDown()
+            assertTrue(stopReturned.await(5, TimeUnit.SECONDS))
+            assertEquals(0, prods.get())
+        } finally {
+            releaseProd.countDown()
+            monitor.stop()
+            stopper.join(5_000)
+        }
     }
 
     @Test