Fixes tunnel startup options handling #489
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
diegoreymendez
commented
Sep 3, 2023
| @@ -65,7 +65,10 @@ public enum ExtensionMessage: RawRepresentable { | |||
| case .isHavingConnectivityIssues: | |||
| self = .isHavingConnectivityIssues | |||
| case .setSelectedServer: | |||
| guard data.count > 1 else { return nil } | |||
There was a problem hiding this comment.
This should not have been ignored. It should have reset the selected server.
diegoreymendez
commented
Sep 3, 2023
Comment on lines
-32
to
-36
| public enum NetworkProtectionOptionValue { | ||
| public static let `true` = "true" as NSString | ||
| } |
There was a problem hiding this comment.
We can send bools as numbers which is the standard.
diegoreymendez
commented
Sep 3, 2023
| @@ -184,19 +190,22 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { | |||
| } | |||
|
|
|||
| private func setKeyValidity(_ interval: TimeInterval?) { | |||
| guard keyValidity != interval, | |||
| let interval = interval else { | |||
There was a problem hiding this comment.
A nil interval wasn't meant to bail out... it's meant to be a request to reset the key validity interval.
diegoreymendez
commented
Sep 3, 2023
Comment on lines
+353
to
+359
| switch options.keyValidity { | ||
| case .set(let validity): | ||
| setKeyValidity(validity) | ||
| case .useExisting: | ||
| break | ||
| case .reset: | ||
| setKeyValidity(nil) |
There was a problem hiding this comment.
Now we have a basic mechanism to know whether we need to set the validity, use the existing one or reset it.
diegoreymendez
commented
Sep 3, 2023
Comment on lines
+365
to
+370
| case .set(let selectedServer): | ||
| selectedServerStore.selectedServer = selectedServer | ||
| case .useExisting: | ||
| break | ||
| case .reset: | ||
| selectedServerStore.selectedServer = .automatic |
There was a problem hiding this comment.
Same as above but for server selection.
diegoreymendez
commented
Sep 3, 2023
Comment on lines
+376
to
+384
| case .set(let authToken): | ||
| try tokenStore.store(authToken) | ||
| case .useExisting: | ||
| break | ||
| case .reset: | ||
| // This case should in theory not be possible, but it's ideal to have this in place | ||
| // in case an error in the controller on the client side allows it. | ||
| try tokenStore.deleteToken() | ||
| throw TunnelError.startingTunnelWithoutAuthToken |
There was a problem hiding this comment.
Same as above but for the auth token.
diegoreymendez
commented
Sep 3, 2023
Comment on lines
-394
to
-399
| var isOnDemand: Bool { | ||
| options?[NetworkProtectionOptionKey.isOnDemand] as? Bool == true | ||
| } | ||
| var isActivatedFromSystemSettings: Bool { | ||
| options?[NetworkProtectionOptionKey.activationAttemptId] == nil && !isOnDemand | ||
| } |
There was a problem hiding this comment.
This is now handled within StartupOptions.
diegoreymendez
commented
Sep 3, 2023
| if error != nil { | ||
| self?.connectionStatus = .disconnected | ||
| completionHandler(error) | ||
| guard let error else { |
There was a problem hiding this comment.
Reverted this condition using a guard so things moved around a bit... sorry!
diegoreymendez
changed the title
diegoreymendez
changed the title
diegoreymendez
commented
Sep 11, 2023
|
|
||
| let internalCompletionHandler = { [weak self] (error: Error?) in | ||
| if error != nil { | ||
| self?.connectionStatus = .disconnected |
There was a problem hiding this comment.
Tis was moved to line 422. The diff in this file is really awful. Apologies!
diegoreymendez
commented
Sep 11, 2023
| @@ -342,32 +369,42 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { | |||
| let vendorOptions = provider?.providerConfiguration | |||
|
|
|||
| loadRoutes(from: vendorOptions) | |||
| self.isConnectionTesterEnabled = vendorOptions?[NetworkProtectionOptionKey.connectionTesterEnabled] as? Bool ?? true | |||
There was a problem hiding this comment.
This should not have been in vendor options. In fact it was not disabled in iOS due to this.
I've moved it to line 362 so it's part of the startup options instead.
graeme
approved these changes
Sep 12, 2023
diegoreymendez
added a commit
to duckduckgo/macos-browser
that referenced
this pull request
Sep 12, 2023
Task/Issue URL: https://app.asana.com/0/0/1205465612515839/f BSK PR: duckduckgo/BrowserServicesKit#489 iOS PR: duckduckgo/iOS#2003 ## Description Fixes startup options reset, and improves the options loading logic so we can easily unit test it.
diegoreymendez
added a commit
to duckduckgo/iOS
that referenced
this pull request
Sep 12, 2023
Network Protection: Connection Tester Task/Issue URL: https://app.asana.com/0/0/1205465612515839/f BSK PR: duckduckgo/BrowserServicesKit#489 macOS PR: duckduckgo/macos-browser#1581 ## Description Fixes startup options reset, and improves the options loading logic so we can easily unit test it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Task/Issue URL: https://app.asana.com/0/0/1205466155643247/f
iOS PR: duckduckgo/iOS#2003
macOS PR: duckduckgo/macos-browser#1581
What kind of version bump will this require?: Patch
Description
Fixes tunnel startup options handling, and improves the options loading mechanism to make it unit testable.
Steps to test this PR
Follow the testing steps in the macOS and iOS PRs.
Internal references:
Software Engineering Expectations
Technical Design Template