VPN-6084: save new server info on iOS, even if disconnected

[mcleinman]

Description

This fixes a long standing bug that will become more apparent when we release App Intents.

If you are disconnected and change servers in the client, we do not push that change to the network extension until the next activation. However, if that next activation comes from somewhere other than the client, it doesn't have that updated info - and so it connects to the prior server. Worse yet, when you open the client, it makes it look like it's connected to the latest server, when really it's connected to that prior one.

Currently, the only way to repro this bug is by turning on the VPN from the control center or system settings. However, App Intents would also activate the bug, so it's time to fix it.

I considered a few ways to fix it, including having the App Intent start the activation from the C++ controller (which would fix it for app intents, but not the pre-existing way described above). Ultimately, this seemed the most straightforward, and leaves fewer future foot guns.

Additionally, this fixes a bug in my initial implementation of App Intents - it would not have created those on demand rules, which are needed to cover up for an Apple-side bug. I've fixed that here.

Reference

VPN-6084

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

[strseb]

Just some small comments :)

[strseb]

Imho that whole flow should not be ifdef'd to iOS - i'd prefer if we had it everywhere but just controllers that don't need that ignore ReasonUpdading.

(android should do that too, we have a similar bug with always on and the tile-widget c: )

[oskirby]

I agree, I want this feature for flatpaks too.

[oskirby]

In fact, I would go even further and make a new ControllerImpl::setConfig() method that does the following:

• If inactive, just sync the updated config into the system settings and return.
• If active, perform a server switch to the new configuration.

[oskirby]

We could go even further further in making the controller align with the backend:

• ControllerImpl::setConfig(const InterfaceConfig& config) - Sets configuration
• ControllerImpl::activate() - Activates using whatever configuration was last set and passes no configuration at all - gets a little tricky when handling multihop though.

[mcleinman]

We could go even further further in making the controller align with the backend:
ControllerImpl::setConfig(const InterfaceConfig& config) - Sets configuration
ControllerImpl::activate() - Activates using whatever configuration was last set and passes no configuration at all - gets a little tricky when handling multihop though.

Agree this is the cleanest method. However, it would require some notification back to the controller that the config was set - because we don't want to activate before it's fully set. At least on mobile, this would require callbacks, and cause more code smell than I think is worthwhile.

However - I'm going to update this to make that setConfig method on ControllerImpl. I'm going to make it no-op on everything except iOS for now.

I'm not going to change how we handle switch servers (when controller is active) for now. Once this is implemented on all platforms, let's revisit it - but for now, that seems like a larger thing than we need to do.

[strseb]

Preamble: This is not the fault of your code, but that piece just made me notice that. But it's fine to keep if you feel like it.

Imho it's a bit akward we need to modify local state to call a function, then check other local state to check what the function produced.

WDYT, split the data generation into a pure func auto Controller::setupConfig(...) static const -> QList<InterfaceConfig>
and have the callee's modify local state.

[mcleinman]

I did a partway improvement - I'm making the callsite handle the activation queue and such, but there are still a couple things that potentially get modified in the setup config function, so it's not a const.

[oskirby]

Just some nits on the API:

1. We don't really need canSendUpdatedConfig(), the default implementation can be a no-op for platforms that don't support it.
2. It would be more efficient to pass the config by reference, eg: virtual void sendUpdatedConfig(const InterfaceConfig& entryConfig, const InterfaceConfig& exitConfig) {};

[mcleinman]

1. I wanted to avoid some future state where a developer doesn't realize that isn't implemented everywhere, and calls it as if it is. Another way of handling this that I considered is just adding a big scary comment in the code around this function that explains the situation. However, I also wanted something in the logs - and unless we make the default implementation a bit chunkier by handling logging, that has to come from controller.cpp - and thus I believe we'd need to have the boolean check so we can log something if it is not implemented. I still prefer the way I did it, but I do realize it adds a little bit of overhead here - if you feel strongly, let me know and I'll remove the boolean and log line, and replace it with some scary warning in a code comment.

2. Good call. Updated.

[oskirby]

I think this looks good