Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes some VPN uninstallation issues #2820

Merged
merged 1 commit into from
May 30, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import Subscription
typealias NetworkProtectionStatusChangeHandler = (NetworkProtection.ConnectionStatus) -> Void
typealias NetworkProtectionConfigChangeHandler = () -> Void

// swiftlint:disable:next type_body_length
final class NetworkProtectionTunnelController: TunnelController, TunnelSessionProvider {

// MARK: - Settings
Expand Down Expand Up @@ -94,6 +95,13 @@ final class NetworkProtectionTunnelController: TunnelController, TunnelSessionPr
///
private var internalManager: NETunnelProviderManager?

/// Simply clears the internal manager so the VPN manager is reloaded next time it's requested.
///
@MainActor
private func clearInternalManager() {
internalManager = nil
}

/// The last known VPN status.
///
/// Should not be used for checking the current status.
Expand Down Expand Up @@ -178,6 +186,7 @@ final class NetworkProtectionTunnelController: TunnelController, TunnelSessionPr

subscribeToSettingsChanges()
subscribeToStatusChanges()
subscribeToConfigurationChanges()
}

// MARK: - Observing Status Changes
Expand Down Expand Up @@ -209,6 +218,31 @@ final class NetworkProtectionTunnelController: TunnelController, TunnelSessionPr
}
}

// MARK: - Observing Configuation Changes

private func subscribeToConfigurationChanges() {
notificationCenter.publisher(for: .NEVPNConfigurationChange)
.receive(on: DispatchQueue.main)
.sink { _ in
Task { @MainActor in
guard let manager = await self.manager else {
return
}

do {
try await manager.loadFromPreferences()

if manager.connection.status == .invalid {
self.clearInternalManager()
}
} catch {
self.clearInternalManager()
}
}
}
.store(in: &cancellables)
}
Comment on lines +223 to +244
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the configuration changes we load it to make sure we have the latest. Or we clear it if it's become invalid.


// MARK: - Subscriptions

private func subscribeToSettingsChanges() {
Expand Down Expand Up @@ -693,6 +727,14 @@ final class NetworkProtectionTunnelController: TunnelController, TunnelSessionPr
func disableOnDemand(tunnelManager: NETunnelProviderManager) async throws {
try await tunnelManager.loadFromPreferences()

guard tunnelManager.connection.status != .invalid else {
// An invalid connection status means the VPN isn't really configured
// so we don't want to save changed because that would re-create the VPN
// configuration.
clearInternalManager()
return
}
Comment on lines +730 to +736
Copy link
Contributor Author

@diegoreymendez diegoreymendez May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the gist of the fix for the issues I was able to reproduce. The tunnel manager's connection becomes invalid if the configuration isn't saved.

In that case we don't really want to save changes, cos that'd prompt the user to create the configuration.


tunnelManager.isOnDemandEnabled = false

try await tunnelManager.saveToPreferences()
Expand Down
Loading