VPN-5681 - remove iOS VPN profile when logging out or resetting #8329

mcleinman · 2023-10-18T23:44:22Z

Description

For several reasons (to make "reset and quit" cleaner, to clearly cut someone off after their subscription ends or they log out) we want to remove the VPN config from iOS System Settings. This does that.

We may want to add this functionality to non-iOS platforms, but I'm leaving that for the future.

Reference

https://mozilla-hub.atlassian.net/browse/VPN-5681
This also fulfills https://mozilla-hub.atlassian.net/browse/VPN-4216

Checklist

My code follows the style guidelines for this project
I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
I have performed a self review of my own code
I have commented my code PARTICULARLY in hard to understand areas
I have added thorough tests where needed

src/mozillavpn.cpp

src/connectionmanager.cpp

MattLichtenstein

long overdue safety net here, nice job!

I think we'll also want to delete the tunnel any time we arrive in App::State::StateInitialize or App::UserState::UserNotAuthenticated. The most notable case I'm thinking of is when a device is deleted from another device and sets those states in MozillaVPN::reset()

src/mozillavpn.cpp

mcleinman · 2023-10-19T18:43:05Z

long overdue safety net here, nice job!

I think we'll also want to delete the tunnel any time we arrive in App::State::StateInitialize or App::UserState::UserNotAuthenticated. The most notable case I'm thinking of is when a device is deleted from another device and sets those states in MozillaVPN::reset()

We start the app inStateInitialize, right? If that's the case, wouldn't connecting StateInitialize to removing the config end up with some bad side effects? (We could mitigate by only destroying it when we move into StateInitialize from another state, but that seems like an antipattern, and is fragile if we ever change the app to start in StatePreInit (and then go into StateInitialize or something like that).

MattLichtenstein · 2023-10-19T18:58:48Z

We start the app inStateInitialize, right? If that's the case, wouldn't connecting StateInitialize to removing the config end up with some bad side effects?

haha yea definitely don't want to remove the config on every app launch. what about App::UserState::UserNotAuthenticated? that seems appropriately related to when we want to remove the config. I wonder if we unauthenticate before quitting on a reset and quit... doubt it though. maybe there is no one size fits all here and we just need to remove the config on a couple of different occasions

src/platforms/ios/ioscontroller.swift

Gela · 2023-10-19T19:21:33Z

Can we add a functional test for this change? Not sure if there's a way to test toggling on the VPN from systems setting but I think we can write a test for the other scenario VPN cannot be turned on on a first attempt after a reset and quit action

mcleinman · 2023-10-19T19:25:26Z

We start the app inStateInitialize, right? If that's the case, wouldn't connecting StateInitialize to removing the config end up with some bad side effects?

haha yea definitely don't want to remove the config on every app launch. what about App::UserState::UserNotAuthenticated? that seems appropriately related to when we want to remove the config. I wonder if we unauthenticate before quitting on a reset and quit... doubt it though. maybe there is no one size fits all here and we just need to remove the config on a couple of different occasions

Just looked at this a bit, and it seems like every use of UserState::UserNotAuthenticated leads to logging out - it seems unneeded here, but if you want to be extra defensive I can add a listener for the userStateChanged signal in ConnectionManager and run the "remove profile" code there.

mcleinman · 2023-10-19T19:28:33Z

src/platforms/ios/ioscontroller.mm

-                   gleanDebugTag:settingsHolder->gleanDebugTagActive()
-                                     ? settingsHolder->gleanDebugTag().toNSString()
-                                     : @""
+      serverIpv6Gateway:config.m_serverIpv6Gateway.toNSString()


this was a linter change when merging main back in

mcleinman · 2023-10-19T19:29:10Z

src/platforms/ios/iostunnelmanager.swift

@@ -47,7 +47,7 @@ class TunnelManager {
        }
    }

-    static func withTunnel(_ f: (_ tunnel: NETunnelProviderManager) throws -> Any) -> Any? {
+    @discardableResult static func withTunnel(_ f: (_ tunnel: NETunnelProviderManager) throws -> Any) -> Any? {


This allows us to ignore the result, which gives some cleaner code where we were using this.

mcleinman · 2023-10-19T19:29:22Z

Can we add a functional test for this change? Not sure if there's a way to test toggling on the VPN from systems setting but I think we can write a test for the other scenario VPN cannot be turned on on a first attempt after a reset and quit action

Good call, working on test now.

Gela · 2023-10-23T21:49:56Z

tests/functional/helper.js

@@ -189,6 +193,14 @@ module.exports = {
        `Command failed: ${json.error}`);
  },

+  // This is used when hitting the "Reset and Quit" button


Can you expand on this comment? e.g. why are we accepting any result

Thanks. Done - let me know if you think we need some more here.

Gela · 2023-10-23T21:52:20Z

tests/functional/helper.js

    }
  },

+  async awaitConnectionOkay() {


Can you add a comment explaining what this helper does?

And possibly come up with a more descriptive name? Something like waitForVPNToActivateSuccessfully()

Thanks. Done and done - let me know if you think we need some more here.

tests/functional/helper.js

Co-authored-by: Gela <[email protected]>

Gela

Wooo! Thanks for figuring out a way to add a test for this too 👌🏽

…ile/mozilla-vpn-client into vpn-5681-clear-ios-vpn-setup

MattLichtenstein

nice job!

question though: our functional test suite doesn't currently run on mobile clients, and these changes are iOS specific. is there any benefit to the added functional test?

tests/functional/testSettings.js

mcleinman · 2023-10-24T16:15:56Z

question though: our functional test suite doesn't currently run on mobile clients, and these changes are iOS specific. is there any benefit to the added functional test?

Correct - this is something I mentioned when Gela and I paired on this yesterday. This doesn't actually run on iOS, but in my opinion the benefits are:

At some point hopefully we'll run functional tests there, and then we'll have this test ready to go.
It helps us stay true to our "always add tests" mission.

Gela · 2023-10-24T16:48:13Z

@MattLichtenstein In addition to what Matt C said, we are also testing the "Reset and Quit" functionality on Desktop in general which I believe wasn't being done previously.

VPN-5681 - remove iOS VPN profile when logging out or resetting

ed16b9e

mcleinman requested review from Gela, MattLichtenstein and lesleyjanenorton October 18, 2023 23:44

Gela reviewed Oct 19, 2023

View reviewed changes

src/mozillavpn.cpp Outdated Show resolved Hide resolved

src/connectionmanager.cpp Show resolved Hide resolved

MattLichtenstein suggested changes Oct 19, 2023

View reviewed changes

src/mozillavpn.cpp Outdated Show resolved Hide resolved

PR feedback

a7c15b0

Gela reviewed Oct 19, 2023

View reviewed changes

src/platforms/ios/ioscontroller.swift Outdated Show resolved Hide resolved

updating name

58ee8ff

mcleinman added 2 commits October 19, 2023 12:25

Merge branch 'main' into vpn-5681-clear-ios-vpn-setup

04fb7d2

Merge branch 'main' into vpn-5681-clear-ios-vpn-setup

d5c8b31

mcleinman commented Oct 19, 2023

View reviewed changes

PR feedback, including test

a7be467

Gela suggested changes Oct 23, 2023

View reviewed changes

mcleinman and others added 2 commits October 23, 2023 15:19

PR feedback

1172b0d

Update tests/functional/helper.js

b2096b9

Co-authored-by: Gela <[email protected]>

Gela approved these changes Oct 23, 2023

View reviewed changes

mcleinman requested a review from MattLichtenstein October 23, 2023 22:27

mcleinman added 2 commits October 23, 2023 16:33

fix tests

2fe822b

Merge branch 'vpn-5681-clear-ios-vpn-setup' of github.com:mozilla-mob…

b53aa2b

…ile/mozilla-vpn-client into vpn-5681-clear-ios-vpn-setup

MattLichtenstein approved these changes Oct 24, 2023

View reviewed changes

MattLichtenstein reviewed Oct 24, 2023

View reviewed changes

tests/functional/testSettings.js Show resolved Hide resolved

fix tests

393bdc6

mcleinman merged commit 35578de into main Oct 24, 2023
85 checks passed

mcleinman deleted the vpn-5681-clear-ios-vpn-setup branch October 24, 2023 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPN-5681 - remove iOS VPN profile when logging out or resetting #8329

VPN-5681 - remove iOS VPN profile when logging out or resetting #8329

mcleinman commented Oct 18, 2023

MattLichtenstein left a comment

mcleinman commented Oct 19, 2023

MattLichtenstein commented Oct 19, 2023

Gela commented Oct 19, 2023

mcleinman commented Oct 19, 2023

mcleinman Oct 19, 2023

mcleinman Oct 19, 2023

mcleinman commented Oct 19, 2023

Gela Oct 23, 2023

mcleinman Oct 23, 2023

Gela Oct 23, 2023

mcleinman Oct 23, 2023

Gela left a comment

MattLichtenstein left a comment

mcleinman commented Oct 24, 2023

Gela commented Oct 24, 2023

VPN-5681 - remove iOS VPN profile when logging out or resetting #8329

VPN-5681 - remove iOS VPN profile when logging out or resetting #8329

Conversation

mcleinman commented Oct 18, 2023

Description

Reference

Checklist

MattLichtenstein left a comment

Choose a reason for hiding this comment

mcleinman commented Oct 19, 2023

MattLichtenstein commented Oct 19, 2023

Gela commented Oct 19, 2023

mcleinman commented Oct 19, 2023

mcleinman Oct 19, 2023

Choose a reason for hiding this comment

mcleinman Oct 19, 2023

Choose a reason for hiding this comment

mcleinman commented Oct 19, 2023

Gela Oct 23, 2023

Choose a reason for hiding this comment

mcleinman Oct 23, 2023

Choose a reason for hiding this comment

Gela Oct 23, 2023

Choose a reason for hiding this comment

mcleinman Oct 23, 2023

Choose a reason for hiding this comment

Gela left a comment

Choose a reason for hiding this comment

MattLichtenstein left a comment

Choose a reason for hiding this comment

mcleinman commented Oct 24, 2023

Gela commented Oct 24, 2023