Preliminary unit tests for key rotation in TunnelManager

[acb-mv]

The first test (rotation timer is started on account set) works, the second one (account timer is stopped on account unset) currently doesn't.

---

This change is 

[linear]

IOS-482 Test key rotation in `TunnelManagerTests`

[rablador]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acb-mv)

---

ios/MullvadVPNTests/DevicesProxy+Stubs.swift line 15 at r1 (raw file):

struct DevicesProxyStub: DeviceHandling {
    let mockDevice = Device(

There is an extension for this on Device, ie. Device.mock(), but it's currently private and resides in DeviceCheckOperationTests. Maybe we could put it somewhere else and resuse it?

---

ios/MullvadVPNTests/TunnelManagerTests.swift line 89 at r1 (raw file):

        await tunnelManager.unsetAccount()
        // This currently fails, as isRunningPeriodicPrivateKeyRotation is not changed.
        XCTAssertEqual(tunnelManager.isRunningPeriodicPrivateKeyRotation, false)

I can't see us setting .isRunningPeriodicPrivateKeyRotation to false anywhere in the code after an account has been unset. cancelPollingKeyRotation() is currently not called from anywhere at all and stopPeriodicPrivateKeyRotation() in only called from AppDelegate.willResignActive(). Looks like you've found a bug. :)

[buggmagnet]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acb-mv and @rablador)

---

ios/MullvadVPNTests/TunnelManagerTests.swift line 89 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I can't see us setting .isRunningPeriodicPrivateKeyRotation to false anywhere in the code after an account has been unset. cancelPollingKeyRotation() is currently not called from anywhere at all and stopPeriodicPrivateKeyRotation() in only called from AppDelegate.willResignActive(). Looks like you've found a bug. :)

Yup, It sounds like a bug, nice catch !

[acb-mv]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPNTests/TunnelManagerTests.swift line 89 at r1 (raw file):

Previously, buggmagnet wrote…

Yup, It sounds like a bug, nice catch !

The flipside of this is that startPeriodicPrivateKeyRotation() is only called from AppDelegate.didBecomeActive(). If we stop key rotation on account unset and start it on account set, we could change the AppDelegate behaviour to start key rotation only if there is an account set.

[buggmagnet]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acb-mv and @rablador)

---

ios/MullvadVPNTests/TunnelManagerTests.swift line 89 at r1 (raw file):

Previously, acb-mv wrote…

The flipside of this is that startPeriodicPrivateKeyRotation() is only called from AppDelegate.didBecomeActive(). If we stop key rotation on account unset and start it on account set, we could change the AppDelegate behaviour to start key rotation only if there is an account set.

The first thing the RotateKeyOperation does is check whether the device is loggedIn so that should be fine

    override func main() {
        // Extract login metadata.
        guard case let .loggedIn(accountData, deviceData) = interactor.deviceState else {
            finish(result: .failure(InvalidDeviceStateError()))
            return
        }

[acb-mv]

Reviewable status: 2 of 6 files reviewed, 2 unresolved discussions (waiting on @rablador)

---

ios/MullvadVPNTests/TunnelManagerTests.swift line 89 at r1 (raw file):

Previously, buggmagnet wrote…

The first thing the RotateKeyOperation does is check whether the device is loggedIn so that should be fine

    override func main() {
        // Extract login metadata.
        guard case let .loggedIn(accountData, deviceData) = interactor.deviceState else {
            finish(result: .failure(InvalidDeviceStateError()))
            return
        }

Testing for this would mean a unit test waiting for the key rotation to actually take place and seeing that it bailed. Would there be an easy way to set the rotation time to something absurdly short for the purpose of unit testing?

[buggmagnet]

Reviewed 1 of 5 files at r1, all commit messages.
Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @acb-mv and @rablador)

---

ios/MullvadVPN/TunnelManager/TunnelManager.swift line 64 at r2 (raw file):

    private var hasAccount = false {
        didSet(previous) {

Couple of things to note :

• We decided not too long ago to not have code anymore in didSet because setters should be side effect free, and because it makes the code harder to follow in general.
• It also has the implication that writing code in setters that means we need to write tests for setters, and that sounds like an architecture problem
• The TunnelManager doesn't really have a notion of an account, and I don't think this is the right place to install one.
• The DeviceState has the notion of an account being set already (via the loggedIn(StoredAccountData, StoredDeviceData) case)
• For testing this, given that we tie whether key rotations are enabled to the lifetime of the UI, have you tried UIApplication.didBecomeActiveNotification in the test to see if it would work ?

[rablador]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @acb-mv and @buggmagnet)

[acb-mv]

Reviewable status: 5 of 331 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/TunnelManager/TunnelManager.swift line 64 at r2 (raw file):

Previously, buggmagnet wrote…

Couple of things to note :

• We decided not too long ago to not have code anymore in didSet because setters should be side effect free, and because it makes the code harder to follow in general.
• It also has the implication that writing code in setters that means we need to write tests for setters, and that sounds like an architecture problem
• The TunnelManager doesn't really have a notion of an account, and I don't think this is the right place to install one.
• The DeviceState has the notion of an account being set already (via the loggedIn(StoredAccountData, StoredDeviceData) case)
• For testing this, given that we tie whether key rotations are enabled to the lifetime of the UI, have you tried UIApplication.didBecomeActiveNotification in the test to see if it would work ?

I have replaced hasAccount with deviceState.isLoggedIn. It seems to work in the tests; I will test it further.

[acb-mv]

Reviewable status: 4 of 333 files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPNTests/DevicesProxy+Stubs.swift line 15 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

There is an extension for this on Device, ie. Device.mock(), but it's currently private and resides in DeviceCheckOperationTests. Maybe we could put it somewhere else and resuse it?

Done.

[acb-mv]

Reviewable status: 4 of 334 files reviewed, 2 unresolved discussions (waiting on @acb-mv and @rablador)

---

ios/MullvadVPN/TunnelManager/TunnelManager.swift line 64 at r2 (raw file):

Previously, acb-mv wrote…

I have replaced hasAccount with deviceState.isLoggedIn. It seems to work in the tests; I will test it further.

Working.

---

ios/MullvadVPNTests/DevicesProxy+Stubs.swift line 15 at r1 (raw file):

Previously, acb-mv wrote…

Done.

Working.

[acb-mv]

Reviewable status: 4 of 334 files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadVPN/TunnelManager/TunnelManager.swift line 64 at r2 (raw file):

Previously, acb-mv wrote…

Working.

Done.

[buggmagnet]

Reviewed 1 of 5 files at r1, 2 of 4 files at r2, 1 of 326 files at r3, 4 of 5 files at r5, 1 of 1 files at r6.
Reviewable status: 10 of 334 files reviewed, 4 unresolved discussions (waiting on @acb-mv)

---

ios/MullvadVPN/TunnelManager/SetAccountOperation.swift line 40 at r6 (raw file):

    // true if this action results in an account being set if successful
    var isConstructive: Bool {

This is not used anymore, we should delete it

---

ios/MullvadVPN/TunnelManager/TunnelManager.swift line 65 at r6 (raw file):

    private var privateKeyRotationTimer: DispatchSourceTimer?
    public private(set) var isRunningPeriodicPrivateKeyRotation = false
    public private(set) var nextKeyRotationDate: Date? = nil

⚠️ Redundant Optional Initialization Violation: Initializing an optional variable with nil is redundant (redundant_optional_initialization)

[buggmagnet]

Reviewed 1 of 2 files at r7, all commit messages.
Reviewable status: 9 of 334 files reviewed, 3 unresolved discussions (waiting on @acb-mv and @rablador)

[github-actions]

🚨 End to end tests failed. Please check the failed workflow run.