Add regression test: Default to connecting state on corrupt state cache

[MarkusPettersson98]

Add regression test which checks that the daemon successfully recovers from a corrupt target state cache.
If the target state cache is corrupt, the daemon should default to the Connecting target state on startup.

Fixes DES-460

---

This change is 

[linear]

DES-460 Default to connecting state when the daemon starts with a corrupt target state cache

Default to connecting when the daemon starts if the target state cache cannot be read or parsed. This behavior was introduced the 2021.1-beta1 release

Link to changelog entry: https://github.com/mullvad/mullvadvpn-app/blob/main/CHANGELOG.md#security-10

[dlon]

Reviewed 3 of 8 files at r1, all commit messages.
Reviewable status: 3 of 9 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

---

mullvad-daemon/src/target_state.rs line 248 at r2 (raw file):

        // A completely blank slate. No target state cache file has been created yet.
        let cache_dir = TestCache::new();
        let target_state = PersistentTargetState::new(&cache_dir).await;

Have you considered not using I/O here? Eg passing a closure or trait to new_inner that calls fs::read_to_string in the real case, but just returns a string in the test cases. I suspect it could make things simpler and more stable.

---

test/test-rpc/src/lib.rs line 155 at r2 (raw file):

        /// Restart the Mullvad VPN application.
        async fn restart_app() -> Result<(), Error>;

Not blocking, but we could probably get rid of set_daemon_service_state with these changes.

---

test/test-rpc/src/lib.rs line 155 at r2 (raw file):

        /// Restart the Mullvad VPN application.
        async fn restart_app() -> Result<(), Error>;

Should these be called ..._mullvad_daemon to clarify that we're not talking about the GUI?

[MarkusPettersson98]

Reviewable status: 3 of 9 files reviewed, all discussions resolved

---

mullvad-daemon/src/target_state.rs line 248 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Have you considered not using I/O here? Eg passing a closure or trait to new_inner that calls fs::read_to_string in the real case, but just returns a string in the test cases. I suspect it could make things simpler and more stable.

Thanks for the suggestion! It did indeed simplify the tests quite a lot :)

---

test/test-rpc/src/lib.rs line 155 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should these be called ..._mullvad_daemon to clarify that we're not talking about the GUI?

Yes, that's a good idea :)

---

test/test-rpc/src/lib.rs line 155 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Not blocking, but we could probably get rid of set_daemon_service_state with these changes.

Will look into it

[dlon]

Reviewed 5 of 8 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

test/test-manager/src/tests/tunnel_state.rs line 352 at r3 (raw file):

    // target cache the user should end up in the connecting/connected state,
    // *not in the disconnected state, upon restart.
    disconnect_and_wait(&mut mullvad_client).await?;

Nit: We can (hopefully) assume that the test starts off in the disconnected state.

---

test/test-manager/src/tests/tunnel_state.rs line 379 at r3 (raw file):

    let new_state = wait_for_tunnel_state(mullvad_client.clone(), |state| {
        matches!(

Nit: How about simply !state.is_disconnected()?

---

test/test-manager/src/tests/tunnel_state.rs line 393 at r3 (raw file):

        matches!(
            new_state,
            TunnelState::Connecting { .. } | TunnelState::Connected { .. }

Nit: How about simply !state.is_disconnected()?

[MarkusPettersson98]

Reviewable status: 4 of 10 files reviewed, all discussions resolved (waiting on @dlon)

---

test/test-manager/src/tests/tunnel_state.rs line 352 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: We can (hopefully) assume that the test starts off in the disconnected state.

Yeah, that's true. Left the comment in place for a bit of background information :)

[dlon]

Reviewed 5 of 6 files at r4, all commit messages.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)

---

test/test-runner/src/sys.rs line 530 at r1 (raw file):

#[cfg(target_os = "windows")]
pub async fn set_mullvad_daemon_service_state(on: bool) -> Result<(), test_rpc::Error> {

Should these not be removed on macOS and Windows too?

[MarkusPettersson98]

Reviewable status: 8 of 10 files reviewed, all discussions resolved (waiting on @dlon)

---

test/test-runner/src/sys.rs line 530 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should these not be removed on macOS and Windows too?

Done 🤦‍♂️

[dlon]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved