diff --git a/app/network/NetworkApp.swift b/app/network/NetworkApp.swift index 637465da..c97d7c00 100644 --- a/app/network/NetworkApp.swift +++ b/app/network/NetworkApp.swift @@ -167,12 +167,14 @@ struct NetworkApp: App { } private func setupDeviceStores(_ device: SdkDeviceRemote) { + let localState = deviceManager.asyncLocalState?.getLocalState() throughputStore.setup(device) - blockActionsStore.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) + blockActionsStore.setup(device, localState: localState) dnsSettingsStore.setup(device) - // the app local state mirrors the transport settings edits so they - // survive an app relaunch while the tunnel is down (see the store) - transportSettingsStore.setup(device, localState: deviceManager.asyncLocalState?.getLocalState()) + transportSettingsStore.setup(device, localState: localState) networkPeersStore.setup(device) reliabilityStore.setup(device) } diff --git a/app/network/Shared/ViewModels/BlockActionsStore.swift b/app/network/Shared/ViewModels/BlockActionsStore.swift index e4c72319..331472d0 100644 --- a/app/network/Shared/ViewModels/BlockActionsStore.swift +++ b/app/network/Shared/ViewModels/BlockActionsStore.swift @@ -154,6 +154,16 @@ private class BlockActionOverridesListener: NSObject, SdkBlockActionOverridesCha } } +private class OverridesRemoteListener: NSObject, SdkRemoteChangeListenerProtocol { + private let callback: (Bool) -> Void + init(callback: @escaping (Bool) -> Void) { + self.callback = callback + } + func remoteChanged(_ remoteConnected: Bool) { + callback(remoteConnected) + } +} + /** * Publishes the live block action window, block stats, and the * block action overrides ("split rules") @@ -170,11 +180,15 @@ class BlockActionsStore: ObservableObject { @Published private(set) var blockedCount: Int = 0 private var device: SdkDeviceRemote? + // the app's own store, which the split rules are mirrored into; the + // extension keeps its copy in a container this process cannot read + private var localState: SdkLocalState? private var blockActionViewController: SdkBlockActionViewController? private var blockActionsSub: SdkSubProtocol? private var blockActionStatsSub: SdkSubProtocol? private var overridesSub: SdkSubProtocol? + private var remoteSub: SdkSubProtocol? /** * the sdk override objects backing `splitRules`, used to rebuild @@ -223,10 +237,13 @@ class BlockActionsStore: ObservableObject { exitAttributionTimer?.invalidate() } - func setup(_ device: SdkDeviceRemote) { + func setup(_ device: SdkDeviceRemote, localState: SdkLocalState?) { reset() self.device = device + // above the guard below: a device whose block action window will not + // open still has split rules to mirror + self.localState = localState guard let blockActionViewController = device.openBlockActionViewController() else { return @@ -251,6 +268,18 @@ class BlockActionsStore: ObservableObject { self?.updateOverrides() } }) + // the override change the extension replays on connect arrives 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 list rather than from this process's guess at it + self.remoteSub = device.add(OverridesRemoteListener { [weak self] remoteConnected in + DispatchQueue.main.async { + if remoteConnected { + self?.updateOverrides() + } + } + }) updateBlockActions() updateBlockStats() @@ -275,6 +304,8 @@ class BlockActionsStore: ObservableObject { blockActionStatsSub = nil overridesSub?.close() overridesSub = nil + remoteSub?.close() + remoteSub = nil if let blockActionViewController { if let device { device.close(blockActionViewController) @@ -284,6 +315,9 @@ class BlockActionsStore: ObservableObject { } blockActionViewController = nil device = nil + // nothing is persisted from here: reset runs on every backgrounding + // and the mirror is already current by then + localState = nil blockActions = [] splitRules = [] @@ -504,7 +538,21 @@ class BlockActionsStore: ObservableObject { } } - private func updateOverrides() { + /** + * Re-reads the rules from the device and, when the read is one this + * process can vouch for as the WHOLE list, mirrors it. + * + * Only two reads qualify: one taken off a connected device, which is the + * extension's own list, and one taken right after an edit made here, + * which is that list plus the edit. 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 a list it never + * read -- which the next connect would apply to the extension as a wipe. + * + * `getConnected()` is GetRemoteConnected; swift's importer strips the + * redundant "Remote". + */ + private func updateOverrides(afterEdit: Bool = false) { guard let device = self.device else { return } @@ -539,6 +587,37 @@ class BlockActionsStore: ObservableObject { if items != splitRules { splitRules = items } + if afterEdit || device.getConnected() { + persistOverrides() + } + } + + /** + * Writes the rules to the app's own local state. + * + * The extension persists its own copy, but into a container this process + * cannot read, so this mirror is what `DeviceManager.initDevice` seeds + * the next device from -- both so the rules render with the tunnel down, + * and so an edit made then is queued against the whole list instead of + * an empty one. + * + * An empty list is written as an empty list and never as nil: nil takes + * the store's delete branch, and a deleted store reads back as "never + * edited", which would leave the extension's rules in place after the + * user removed the last one. + */ + private func persistOverrides() { + guard let localState = self.localState, let list = SdkBlockActionOverrideList() else { + return + } + for sdkOverride in sdkOverrides { + list.add(sdkOverride) + } + do { + try localState.setBlockActionOverrides(list) + } catch { + print("[BlockActionsStore]failed to persist split rules: \(error.localizedDescription)") + } } /** @@ -564,7 +643,7 @@ class BlockActionsStore: ObservableObject { override.hosts = arrayToStringList(hosts) override.routeOverride = mode.toSdkRouteOverride() device.add(override) - updateOverrides() + updateOverrides(afterEdit: true) } /** @@ -588,7 +667,7 @@ class BlockActionsStore: ObservableObject { list?.add(sdkOverride) } device.setBlockActionOverrides(list) - updateOverrides() + updateOverrides(afterEdit: true) } func removeRule(id: String) { @@ -599,7 +678,7 @@ class BlockActionsStore: ObservableObject { return } device.removeBlockActionOverride(override.overrideId) - updateOverrides() + updateOverrides(afterEdit: true) } private func stringListToArray(_ list: SdkStringList?) -> [String] { diff --git a/app/network/Shared/ViewModels/DeviceManager.swift b/app/network/Shared/ViewModels/DeviceManager.swift index 94ac8408..0936ef89 100644 --- a/app/network/Shared/ViewModels/DeviceManager.swift +++ b/app/network/Shared/ViewModels/DeviceManager.swift @@ -1135,6 +1135,20 @@ extension DeviceManager { device.setProviderTransportSettings(providerTransportSettings) } + // split rules ("block action overrides") cross the same way, mirrored + // by BlockActionsStore. One consequence beyond the transport settings + // above: without the seed the remote rebuilds its "full" list from + // nothing, so an edit made while the tunnel was down does not merely + // fail to save -- it replaces the extension's saved rules on the next + // connect. Nothing stored means never edited, and a store that cannot + // be read reports the same way, so both leave the extension's rules + // alone; an empty list is a real value -- the last rule was deleted -- + // and is seeded as one. This must stay above setDevice, which is what + // publishes the device and drives BlockActionsStore.setup. + if let blockActionOverrides = localState.getBlockActionOverrides() { + device.setBlockActionOverrides(blockActionOverrides) + } + self.setDevice(device: device) return true } diff --git a/app/networkTests/BlockActionOverridesMirrorTests.swift b/app/networkTests/BlockActionOverridesMirrorTests.swift new file mode 100644 index 00000000..411d3f8a --- /dev/null +++ b/app/networkTests/BlockActionOverridesMirrorTests.swift @@ -0,0 +1,164 @@ +// +// BlockActionOverridesMirrorTests.swift +// networkTests +// +// Covers the store the split rules are mirrored into. +// +// The rules 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 sheet renders from with the tunnel down and what +// DeviceManager.initDevice seeds the next device from. +// +// Two of these cases are the ones 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 removed their last rule" and "this +// install has never mirrored" are the same value unless an empty list is +// written as an empty list. The first would resurrect deleted rules on the +// next connect; the second would push an empty list over rules the extension +// still holds. Everything BlockActionsStore.persistOverrides and the seed's +// `if let` do rests on that distinction. +// + +import Testing +import Foundation +import URnetworkSdk +@testable import URnetwork + +struct BlockActionOverridesMirrorTests { + + /// 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("BlockActionOverridesMirrorTests-\(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) + } + + private func override( + hosts: [String], + local: Bool = false, + pin: Bool = false + ) -> SdkBlockActionOverride { + let override = SdkBlockActionOverride() + override.overrideId = SdkNewId() + let hostList = SdkStringList() + for host in hosts { + hostList?.add(host) + } + override.hosts = hostList + let routeOverride = SdkRouteOverride() + routeOverride.local = local + routeOverride.pin = pin + override.routeOverride = routeOverride + return override + } + + private func list(_ overrides: [SdkBlockActionOverride]) -> SdkBlockActionOverrideList { + let list = SdkBlockActionOverrideList() + for override in overrides { + list?.add(override) + } + return list! + } + + private func hosts(_ override: SdkBlockActionOverride) -> [String] { + guard let hosts = override.hosts else { + return [] + } + return (0..