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

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

Merged
merged 11 commits into from
Oct 24, 2023
6 changes: 6 additions & 0 deletions src/connectionmanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,12 @@ void ConnectionManager::quit() {
}
}

void ConnectionManager::deleteOSTunnelConfig() {
if (m_impl) {
m_impl->deleteOSTunnelConfig();
}
}

Gela marked this conversation as resolved.
Show resolved Hide resolved
void ConnectionManager::handshakeTimeout() {
logger.debug() << "Timeout while waiting for handshake";

Expand Down
1 change: 1 addition & 0 deletions src/connectionmanager.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ class ConnectionManager : public QObject, public LogSerializer {
bool switchServers(const ServerData& serverData);
void backendFailure();
void updateRequired();
void deleteOSTunnelConfig();

const ServerData& currentServer() const { return m_serverData; }

Expand Down
5 changes: 5 additions & 0 deletions src/controllerimpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ class ControllerImpl : public QObject {
// "disconnecting" state until the "disconnected" signal is received.
virtual void deactivate(ConnectionManager::Reason reason) = 0;

// This method is used to remove the tunnel config from the operating
// system. This is used upon logging out and resetting, so the
// tunnel cannot be reactivated in system settings.
virtual void deleteOSTunnelConfig(){};

// This method is used to retrieve the VPN tunnel status (mainly the number
// of bytes sent and received). It's called always when the VPN tunnel is
// active.
Expand Down
5 changes: 5 additions & 0 deletions src/mozillavpn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -834,6 +834,8 @@ void MozillaVPN::logout() {
ProductsHandler::instance()->stopProductsRegistration();
}

connectionManager()->deleteOSTunnelConfig();

// update-required state is the only one we want to keep when logging out.
if (state() != StateUpdateRequired) {
setState(StateInitialize);
Expand Down Expand Up @@ -861,6 +863,8 @@ void MozillaVPN::reset(bool forceInitialState) {
m_private->m_keys.forgetKeys();
m_private->m_serverData.forget();

connectionManager()->deleteOSTunnelConfig();

PurchaseHandler::instance()->stopSubscription();
if (!Feature::get(Feature::Feature_webPurchase)->isSupported()) {
ProductsHandler::instance()->stopProductsRegistration();
Expand Down Expand Up @@ -1292,6 +1296,7 @@ void MozillaVPN::hardReset() {
SettingsHolder* settingsHolder = SettingsHolder::instance();
Q_ASSERT(settingsHolder);
settingsHolder->hardReset();
connectionManager()->deleteOSTunnelConfig();
}

void MozillaVPN::hardResetAndQuit() {
Expand Down
2 changes: 2 additions & 0 deletions src/platforms/ios/ioscontroller.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ class IOSController final : public ControllerImpl {

void deactivate(ConnectionManager::Reason reason) override;

void deleteOSTunnelConfig() override;

void checkStatus() override;

void getBackendLogs(std::function<void(const QString&)>&& callback) override;
Expand Down
38 changes: 20 additions & 18 deletions src/platforms/ios/ioscontroller.mm
Original file line number Diff line number Diff line change
Expand Up @@ -142,25 +142,25 @@
Q_ASSERT(settingsHolder);

[impl connectWithDnsServer:config.m_dnsServer.toNSString()
serverIpv6Gateway:config.m_serverIpv6Gateway.toNSString()
serverPublicKey:config.m_serverPublicKey.toNSString()
serverIpv4AddrIn:config.m_serverIpv4AddrIn.toNSString()
serverPort:config.m_serverPort
allowedIPAddressRanges:allowedIPAddressRangesNS
reason:reason
gleanDebugTag:settingsHolder->gleanDebugTagActive()
? settingsHolder->gleanDebugTag().toNSString()
: @""
serverIpv6Gateway:config.m_serverIpv6Gateway.toNSString()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was a linter change when merging main back in

serverPublicKey:config.m_serverPublicKey.toNSString()
serverIpv4AddrIn:config.m_serverIpv4AddrIn.toNSString()
serverPort:config.m_serverPort
allowedIPAddressRanges:allowedIPAddressRangesNS
reason:reason
gleanDebugTag:settingsHolder->gleanDebugTagActive()
? settingsHolder->gleanDebugTag().toNSString()
: @""
isSuperDooperFeatureActive:Feature::get(Feature::Feature_superDooperMetrics)->isSupported()
installationId:config.m_installationId.toNSString()
isOnboarding:MozillaVPN::instance()->state() == App::StateOnboarding
disconnectCallback:^() {
logger.error() << "IOSSWiftController - disconnecting";
emit disconnected();
}
onboardingCompletedCallback:^() {
MozillaVPN::instance()->onboardingCompleted();
}];
installationId:config.m_installationId.toNSString()
isOnboarding:MozillaVPN::instance()->state() == App::StateOnboarding
disconnectCallback:^() {
logger.error() << "IOSSWiftController - disconnecting";
emit disconnected();
}
onboardingCompletedCallback:^() {
MozillaVPN::instance()->onboardingCompleted();
}];
}

void IOSController::deactivate(ConnectionManager::Reason reason) {
Expand All @@ -181,6 +181,8 @@
[impl disconnect];
}

void IOSController::deleteOSTunnelConfig() { [impl deleteOSTunnelConfig]; }

void IOSController::checkStatus() {
logger.debug() << "Checking status";

Expand Down
17 changes: 14 additions & 3 deletions src/platforms/ios/ioscontroller.swift
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public class IOSControllerImpl : NSObject {
@objc func connect(dnsServer: String, serverIpv6Gateway: String, serverPublicKey: String, serverIpv4AddrIn: String, serverPort: Int, allowedIPAddressRanges: Array<VPNIPAddressRange>, reason: Int, gleanDebugTag: String, isSuperDooperFeatureActive: Bool, installationId: String, isOnboarding: Bool, disconnectCallback: @escaping () -> Void, onboardingCompletedCallback: @escaping () -> Void) {
IOSControllerImpl.logger.debug(message: "Connecting")

let _ = TunnelManager.withTunnel { tunnel in
TunnelManager.withTunnel { tunnel in
// Let's remove the previous config if it exists.
(tunnel.protocolConfiguration as? NETunnelProviderProtocol)?.destroyConfigurationReference()

Expand Down Expand Up @@ -157,7 +157,7 @@ public class IOSControllerImpl : NSObject {
}

func configureTunnel(config: TunnelConfiguration, reason: Int, serverName: String, gleanDebugTag: String, isSuperDooperFeatureActive: Bool, installationId: String, isOnboarding: Bool, disconnectCallback: @escaping () -> Void, onboardingCompletedCallback: @escaping () -> Void) {
let _ = TunnelManager.withTunnel { tunnel in
TunnelManager.withTunnel { tunnel in
let proto = NETunnelProviderProtocol(tunnelConfiguration: config)
proto!.providerBundleIdentifier = TunnelManager.vpnBundleId
proto!.disconnectOnSleep = false
Expand Down Expand Up @@ -238,10 +238,21 @@ public class IOSControllerImpl : NSObject {
TunnelManager.session?.stopTunnel()
}

@objc func deleteOSTunnelConfig() {
IOSControllerImpl.logger.info(message: "Removing tunnel from iOS System Preferences")
TunnelManager.withTunnel { tunnel in
tunnel.removeFromPreferences(completionHandler: { error in
if let error = error {
IOSControllerImpl.logger.info(message: "Error when removing tunnel \(error.localizedDescription)")
}
})
}
}

@objc func checkStatus(callback: @escaping (String, String, String) -> Void) {
IOSControllerImpl.logger.info(message: "Check status")

let _ = TunnelManager.withTunnel { tunnel in
TunnelManager.withTunnel { tunnel in
let proto = tunnel.protocolConfiguration as? NETunnelProviderProtocol
if proto == nil {
callback("", "", "")
Expand Down
2 changes: 1 addition & 1 deletion src/platforms/ios/iostunnelmanager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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? {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

guard let tunnel = TunnelManager.instance.tunnel else {
logger.error(message: "Attempted to use the VPN tunnel, but it's not available.")
return nil
Expand Down
2 changes: 2 additions & 0 deletions src/ui/screens/getHelp/developerMenu/ViewDeveloperMenu.qml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import components 0.1
import components.forms 0.1

MZViewBase {
objectName: "developerScreen"
_menuTitle: MZI18n.SettingsDevTitle
_viewContentData: ColumnLayout {
id: root
Expand Down Expand Up @@ -257,6 +258,7 @@ MZViewBase {

MZButton {
id: resetAndQuit
objectName: "resetAndQuitButton"
property int clickNeeded: 5

text: "Reset and Quit"
Expand Down
29 changes: 22 additions & 7 deletions tests/functional/helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,16 +76,21 @@ module.exports = {
`Command failed: ${json.error}`);

if (awaitConnectionOkay) {
await this.waitForCondition(async () => {
let title = await this.getQueryProperty(
queries.screenHome.CONTROLLER_TITLE.visible(), 'text');
let unsettled = await this.getMozillaProperty(
'Mozilla.VPN', 'VPNConnectionHealth', 'unsettled');
return (title == 'VPN is on') && (unsettled == 'false');
});
awaitSuccessfulConnection();
}
},

// Waits for VPN connection to be active and healthy.
async awaitSuccessfulConnection() {
await this.waitForCondition(async () => {
let title = await this.getQueryProperty(
queries.screenHome.CONTROLLER_TITLE.visible(), 'text');
let unsettled = await this.getMozillaProperty(
'Mozilla.VPN', 'VPNConnectionHealth', 'unsettled');
return (title == 'VPN is on') && (unsettled == 'false');
});
},

async deactivate() {
const json = await this._writeCommand('deactivate');
assert(
Expand Down Expand Up @@ -189,6 +194,16 @@ module.exports = {
`Command failed: ${json.error}`);
},

// This is used when hitting the "Reset and Quit" button. We expect empty
// JSON object back, so clickOnQuery would never return the `click` in json,
// and command would fail.
async clickOnQueryAndAcceptAnyResults(id) {
assert(await this.query(id), 'Element does not exist.');
const command = `click ${encodeURIComponent(id)}`;

const json = await this._writeCommand(`click ${encodeURIComponent(id)}`);
},

async waitForQueryAndClick(id) {
await this.waitForQuery(id);
await this.clickOnQuery(id);
Expand Down
7 changes: 7 additions & 0 deletions tests/functional/queries.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ const screenGetHelp = {
HELP_CENTER: new QmlQueryComposer('//helpCenter'),
LINKS: new QmlQueryComposer('//getHelpLinks'),
LOGS: new QmlQueryComposer('//viewLogs'),
DEVELOPER_MENU: new QmlQueryComposer('//developer'),
STACKVIEW: new QmlQueryComposer('//getHelpStackView'),
SUPPORT: new QmlQueryComposer('//inAppSupport'),

Expand All @@ -228,6 +229,11 @@ const screenGetHelp = {
}
};

const screenDeveloperMenu = {
SCREEN: new QmlQueryComposer('//developerScreen-flickable'),
RESET_AND_QUIT_BUTTON: new QmlQueryComposer('//resetAndQuitButton'),
};

const appExclusionsView = {
ADD_APPLICATION_BUTTON: new QmlQueryComposer('//addApplication'),
APP_LIST: new QmlQueryComposer('//appList'),
Expand Down Expand Up @@ -484,6 +490,7 @@ module.exports = {
screenTelemetry,
screenAuthenticationInApp,
screenAuthenticating,
screenDeveloperMenu,
screenGetHelp,
screenSettings,
screenBackendFailure,
Expand Down
10 changes: 10 additions & 0 deletions tests/functional/setupVpn.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,17 @@ async function startAndConnect() {
await vpn.connect(vpnWS, {hostname: '127.0.0.1'});
}

function vpnIsInactive() {
try {
vpnProcess.kill(pid, 0);
return false;
} catch (e) {
return true;
}
}

exports.startAndConnect = startAndConnect;
exports.vpnIsInactive = vpnIsInactive;

exports.mochaHooks = {
async beforeAll() {
Expand Down
46 changes: 46 additions & 0 deletions tests/functional/testSettings.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
const assert = require('assert');
const queries = require('./queries.js');
const vpn = require('./helper.js');
const setup = require('./setupVpn.js');

describe('Settings', function() {
this.timeout(60000);
Expand Down Expand Up @@ -808,4 +809,49 @@ describe('Settings', function() {
await vpn.clickOnQuery(queries.screenSettings.SIGN_OUT.visible());
await vpn.waitForInitialView();
});

it('Checking Developer Menu Reset and Quit', async () => {
// magically unlock dev menu
await vpn.setSetting('developerUnlock', 'true');

// navigate to Developer Menu
await getToGetHelpView();
await vpn.waitForQueryAndClick(
queries.screenGetHelp.DEVELOPER_MENU.visible());

// click "reset and quit" 6 times, test will fail if app quits early
await vpn.waitForQuery(
queries.screenDeveloperMenu.RESET_AND_QUIT_BUTTON.visible());
await vpn.scrollToQuery(
queries.screenDeveloperMenu.SCREEN,
queries.screenDeveloperMenu.RESET_AND_QUIT_BUTTON);
await vpn.waitForQueryAndClick(
queries.screenDeveloperMenu.RESET_AND_QUIT_BUTTON.visible());
await vpn.waitForQueryAndClick(
queries.screenDeveloperMenu.RESET_AND_QUIT_BUTTON.visible());
await vpn.waitForQueryAndClick(
queries.screenDeveloperMenu.RESET_AND_QUIT_BUTTON.visible());
await vpn.waitForQueryAndClick(
queries.screenDeveloperMenu.RESET_AND_QUIT_BUTTON.visible());
await vpn.waitForQueryAndClick(
queries.screenDeveloperMenu.RESET_AND_QUIT_BUTTON.visible());
// can't use waitForQueryAndClick for final click because it returns an
// error - as expected, we crashed the app - but that causes test to fail
await vpn.clickOnQueryAndAcceptAnyResults(
queries.screenDeveloperMenu.RESET_AND_QUIT_BUTTON.visible());

// Confirm the app quit
assert.equal(setup.vpnIsInactive(), true);

// relaunch app
await setup.startAndConnect();
MattLichtenstein marked this conversation as resolved.
Show resolved Hide resolved
await vpn.setSetting('tipsAndTricksIntroShown', 'true')
await vpn.setSetting('localhostRequestsOnly', 'true');
await vpn.authenticateInApp(true, true);
await vpn.setGleanAutomationHeader();

// turn on VPN
await vpn.activateViaToggle();
await vpn.awaitSuccessfulConnection();
});
});
Loading