iOS VPN: ViewController Architecture Problem

[ailinykh]

I made a review of the iOS App and there are some thoughts I would like to share with the community.

The ViewController is responsible for a lot of things:

• UI updates
• network extension configuration
• network extension state management
• observing application lifecycle events
• observing network state

This approach violates the Single Responsibility Principle and can lead to a Massive View Controller problem.

Let's see more precisely a ViewController call order:

graph TD
    A[viewDidLoad] -->|animated:false| B(updateLayout)
    A --> C[update]
    L[toggleTapped] --> E
    L -->|animated:true| B
    M[monitor.pathUpdateHandler] --> C
    A --> D{toggleState}
    D -->|true| E[stateChanged]
    A --> F[wormhole.ListenMessage]
    F --> G[updateStatistic]
    C -->|loadAllFromPreferences| B
    E -->|loadAllFromPreferences| C
    E --> H[enforceState]
    H --> I{toggleState}
    I -->|true| J[startVPNTunnel]
    I -->|false| K[stopVPNTunnel]
    N[foreground] --> E
    subgraph Application Lifecycle Events
        A
        N
    end
    subgraph  External Network and User Touch Events
        L
        M
    end
    subgraph  UI Updates
        B
        C
        G
    end

You can see that some functions are calling another multiple times:

• ViewDidLoad -> updateLayout
• ViewDidLoad -> update -> NETunnelProviderManager.loadAllFromPreferences -> updateLayout

or stateChanged invokes NETunnelProviderManager.loadAllFromPreferences and in the callback block, the update method also invokes the NETunnelProviderManager.loadAllFromPreferences

This can lead to some wired UI bugs like so:

new-node-bug.mp4

Solution

I suggest applying a Separation of concerns design principle and splitting ViewController to 4 components:

• NetworkExtensionManager (holds all the networking stuff, can be divided into small pieces in the future)
• ViewModel (contains all the business logic)
• ViewController (responsible for UI and application lifecycle updates only)
• Presenter (navigation between screens)

I can implement all of this, but I need the approval of the maintainers to make sure we have the same view of things.

[ghazel]

That UI oddity is possibly something more intentional than a bug. It's confusing because of the way the UI appears.

NewNode has an internal state of whether it is "enabled" or not. This is the user preference. It used to be represented with a toggle switch, which would only move when the user tapped it.

Independently, the OS has a notion of whether the VPN is "in use", represented as "Connected"/"Disconnected" in the UI.

So, it makes sense to have NewNode "enabled" but "disconnected", meaning the user wants to use NewNode when possible, but right now it's not possible.

(There's a further state one might call "Connected", regarding whether NewNode can reach peers, but this is not represented in the button or the status message.)

[ailinykh]

I think we could fix it by adding a double-check for the VPN switch enabled in preferences.
The scenario is following:

• User turns on the NewNode VPN
• User switches to another VPN and turns it on
• The NewNode VPN manager in preferences becomes disabled
• User switches back to NewNode VPN
• The .willEnterForegroundNotification performs connect
• startVPNTunnel throws the .configurationDisabled error
• Re-enable the manager and save it to preferences
• Call startVPNTunnel again