diff --git a/app/network/NetworkApp.swift b/app/network/NetworkApp.swift index c97d7c00..1a8203b0 100644 --- a/app/network/NetworkApp.swift +++ b/app/network/NetworkApp.swift @@ -169,11 +169,11 @@ struct NetworkApp: App { private func setupDeviceStores(_ device: SdkDeviceRemote) { let localState = deviceManager.asyncLocalState?.getLocalState() throughputStore.setup(device) - // the app local state mirrors the split rule and transport settings - // edits so they survive an app relaunch while the tunnel is down - // (see the stores) + // the app local state mirrors the split rule, dns and transport + // settings edits so they survive an app relaunch while the tunnel is + // down (see the stores) blockActionsStore.setup(device, localState: localState) - dnsSettingsStore.setup(device) + dnsSettingsStore.setup(device, localState: localState) transportSettingsStore.setup(device, localState: localState) networkPeersStore.setup(device) reliabilityStore.setup(device) diff --git a/app/network/Shared/ViewModels/DeviceManager.swift b/app/network/Shared/ViewModels/DeviceManager.swift index f4f691b5..f84b8273 100644 --- a/app/network/Shared/ViewModels/DeviceManager.swift +++ b/app/network/Shared/ViewModels/DeviceManager.swift @@ -1238,6 +1238,19 @@ extension DeviceManager { device.setBlockActionOverrides(blockActionOverrides) } + // the dns resolver settings cross the same way, mirrored by + // DnsSettingsStore. The editor opens on whatever the device reports, so + // without the seed a tunnel-down launch shows the blank + // never-configured form over the resolver the extension has on disk, + // and applying an edit from that blank base replaces it on the next + // connect. Nothing stored means never mirrored -- leave the + // extension's resolver alone; settings with everything turned off are + // a real value and are seeded as one. Above setDevice for the same + // reason as the rules. + if let dnsResolverSettings = localState.getDnsResolverSettings() { + device.setDnsResolverSettings(dnsResolverSettings) + } + self.setDevice(device: device) return true } diff --git a/app/network/Shared/ViewModels/DnsSettingsStore.swift b/app/network/Shared/ViewModels/DnsSettingsStore.swift index 2b9c7f0a..4f8642a5 100644 --- a/app/network/Shared/ViewModels/DnsSettingsStore.swift +++ b/app/network/Shared/ViewModels/DnsSettingsStore.swift @@ -113,8 +113,27 @@ private class DnsResolverSettingsListener: NSObject, SdkDnsResolverSettingsChang } } +private class DnsSettingsRemoteListener: NSObject, SdkRemoteChangeListenerProtocol { + private let callback: (Bool) -> Void + init(callback: @escaping (Bool) -> Void) { + self.callback = callback + } + func remoteChanged(_ remoteConnected: Bool) { + callback(remoteConnected) + } +} + /** - * Publishes the device dns resolver settings and applies edits + * Publishes the device dns resolver settings and applies edits. + * + * The device persists the settings in its own local state, but in the network + * extension's container, which the app process cannot read. The store mirrors + * every reading it can vouch for into the app's own local state, and + * `DeviceManager.initDevice` seeds the next device from that mirror. Without + * it the editor opens on the blank never-configured form whenever the tunnel + * is down -- and an edit applied from that blank base is not merely lost with + * the app process, it replaces the resolver the extension still holds on the + * next connect. */ @MainActor class DnsSettingsStore: ObservableObject { @@ -122,17 +141,34 @@ class DnsSettingsStore: ObservableObject { @Published private(set) var settings: DnsSettings? = nil private var device: SdkDeviceRemote? + // the app's own store, which the resolver settings are mirrored into; the + // extension keeps its copy in a container this process cannot read + private var localState: SdkLocalState? private var settingsSub: SdkSubProtocol? + private var remoteSub: SdkSubProtocol? - func setup(_ device: SdkDeviceRemote) { + func setup(_ device: SdkDeviceRemote, localState: SdkLocalState?) { reset() self.device = device + self.localState = localState self.settingsSub = device.add(DnsResolverSettingsListener { [weak self] in DispatchQueue.main.async { self?.update() } }) + // the settings the extension replays on connect arrive over the + // reverse sync, which runs before the remote publishes its service, so + // that read still comes out of the remote's own memory. Re-read once + // the rpc is up so the mirror is written from the extension's resolver + // rather than from this process's guess at it + self.remoteSub = device.add(DnsSettingsRemoteListener { [weak self] remoteConnected in + DispatchQueue.main.async { + if remoteConnected { + self?.update() + } + } + }) update() } @@ -140,26 +176,77 @@ class DnsSettingsStore: ObservableObject { func reset() { settingsSub?.close() settingsSub = nil + remoteSub?.close() + remoteSub = nil device = nil + // nothing is persisted from here: reset runs on every backgrounding + // and the mirror is already current by then + localState = nil settings = nil } - private func update() { + /** + * Re-reads the settings from the device and, when the read is one this + * process can vouch for, mirrors it. + * + * Only two reads qualify: one taken off a connected device, which is the + * extension's own resolver, and one taken right after an edit made here. + * With the rpc down and nothing seeded the device answers out of its own + * empty memory instead, and mirroring that would replace the app's only + * durable copy with settings it never read. + * + * The mirror is written from the read-back and never from the value just + * pushed: `DeviceLocal.SetDnsResolverSettings` returns without persisting + * when the settings are nil or the mux is disabled, so the app cannot take + * its own write as accepted. Connected, the read-back is the extension's + * answer to the edit -- a dropped edit mirrors as the settings still in + * force; disconnected, it is the edit the remote queued, which is what the + * next connect applies. + * + * `getConnected()` is GetRemoteConnected; swift's importer strips the + * redundant "Remote". + */ + private func update(afterEdit: Bool = false) { guard let device = self.device else { return } if let sdkSettings = device.getDnsResolverSettings() { settings = DnsSettings(sdkSettings) + if afterEdit || device.getConnected() { + persist(sdkSettings) + } } else { settings = nil } } + /** + * Writes the settings to the app's own local state. + * + * Always the sdk object read back from the device rather than the + * `DnsSettings` projection, so the fields this app does not model (the + * upgrade mask address) survive the mirror. + * + * nil is never written: it takes the store's delete branch, and a deleted + * store reads back as never configured, which is what + * `DeviceManager.initDevice` reads as "leave the extension's resolver + * alone". A device that reports no settings therefore mirrors nothing, + * while settings with everything turned off are a real value -- the user + * emptied the form -- and are mirrored as one. + */ + private func persist(_ sdkSettings: SdkDnsResolverSettings) { + do { + try localState?.setDnsResolverSettings(sdkSettings) + } catch { + print("[DnsSettingsStore]failed to persist dns resolver settings: \(error.localizedDescription)") + } + } + func apply(_ newSettings: DnsSettings) { guard let device = self.device else { return } device.setDnsResolverSettings(newSettings.toSdk()) - update() + update(afterEdit: true) } } diff --git a/app/networkTests/DnsResolverSettingsMirrorTests.swift b/app/networkTests/DnsResolverSettingsMirrorTests.swift new file mode 100644 index 00000000..a9111e18 --- /dev/null +++ b/app/networkTests/DnsResolverSettingsMirrorTests.swift @@ -0,0 +1,125 @@ +// +// DnsResolverSettingsMirrorTests.swift +// networkTests +// +// Covers the store the dns resolver settings are mirrored into. +// +// The settings themselves live in the network extension's container, which +// the app process cannot read, so this mirror is the app's only durable copy +// -- it is what the editor opens on with the tunnel down and what +// DeviceManager.initDevice seeds the next device from. +// +// The distinction the fix would be silently wrong without: writing nil +// DELETES the store, and a deleted store reads back exactly like one that was +// never written, so "the user turned everything off" and "this install has +// never mirrored" are the same value unless an emptied form is written as a +// settings object. Read as never-mirrored it would restore the resolver the +// user just cleared; read as emptied it would push a blank resolver over the +// one the extension still holds. DnsSettingsStore.persist and the seed's +// `if let` both rest on that line. +// + +import Testing +import Foundation +import URnetworkSdk +@testable import URnetwork + +struct DnsResolverSettingsMirrorTests { + + /// Each test gets its own storage home; the SDK roots the store at + /// `/.by`, so a fresh temporary directory is a fresh store. + private func withLocalState(_ body: (SdkLocalState) throws -> Void) rethrows { + let home = FileManager.default.temporaryDirectory + .appendingPathComponent("DnsResolverSettingsMirrorTests-\(UUID().uuidString)") + try? FileManager.default.createDirectory(at: home, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: home) } + + guard let asyncLocalState = SdkAsyncLocalState(home.path()), + let localState = asyncLocalState.getLocalState() else { + Issue.record("could not open a local state under \(home.path())") + return + } + defer { asyncLocalState.close() } + try body(localState) + } + + /// a configuration with both a toggle set and servers in more than one + /// list, so a round trip that drops either shows up + private var configured: DnsSettings { + var settings = DnsSettings() + settings.enableRemoteDoh = true + settings.enableLocalDns = true + settings.enableFallback = true + settings.remoteDohUrlsIpv4 = ["https://1.1.1.1/dns-query", "https://8.8.8.8/dns-query"] + settings.remoteDohUrlsIpv6 = ["https://[2606:4700:4700::1111]/dns-query"] + settings.localDnsIpv4 = ["192.168.1.1"] + settings.localDnsIpv6 = ["fe80::1"] + return settings + } + + /// Every field the editor writes and the connect drawer summarizes -- + /// settings that round-trip without them render as a resolver the user + /// never configured. + @Test func settingsRoundTripWithTheirTogglesAndServers() throws { + try withLocalState { localState in + let configured = self.configured + + try localState.setDnsResolverSettings(configured.toSdk()) + + let read = try #require(localState.getDnsResolverSettings()) + #expect(DnsSettings(read) == configured) + } + } + + /// The mirror copies the device's own settings object rather than the + /// DnsSettings projection the editor renders, so fields the app never + /// models have to survive it too. + @Test func fieldsTheAppDoesNotModelSurviveTheRoundTrip() throws { + try withLocalState { localState in + let sdkSettings = configured.toSdk() + sdkSettings.dnsUpgradeMaskAddress = "10.64.0.1" + + try localState.setDnsResolverSettings(sdkSettings) + + let read = try #require(localState.getDnsResolverSettings()) + #expect(read.dnsUpgradeMaskAddress == "10.64.0.1") + } + } + + /// "I turned everything off" -- an emptied form is a real value and has to + /// come back as one, or the seed skips it and the extension's resolver + /// stands. + @Test func settingsWithNothingEnabledRoundTripAsAValue() throws { + try withLocalState { localState in + try localState.setDnsResolverSettings(configured.toSdk()) + try localState.setDnsResolverSettings(DnsSettings().toSdk()) + + let read = try #require(localState.getDnsResolverSettings()) + #expect(DnsSettings(read) == DnsSettings()) + } + } + + /// Writing nil is the delete branch. DnsSettingsStore.persist must never + /// reach it: the result is indistinguishable from an install that never + /// mirrored. + @Test func writingNilDeletesTheStore() throws { + try withLocalState { localState in + try localState.setDnsResolverSettings(configured.toSdk()) + #expect(localState.getDnsResolverSettings() != nil) + + try localState.setDnsResolverSettings(nil) + + #expect(localState.getDnsResolverSettings() == nil) + } + } + + /// The upgrade guard. A build that has never mirrored reads nil, so + /// DeviceManager.initDevice queues nothing and the resolver a pre-fix build + /// left in the extension is still there on the first launch after this + /// ships -- rather than being replaced by a blank one. + @Test func aStoreThatWasNeverWrittenReadsNilRatherThanEmptySettings() throws { + try withLocalState { localState in + #expect(localState.getDnsResolverSettings() == nil) + } + } +}