Handle legacy always-on vpn profiles

[Rawa]

This PR reworks how we prepare the VpnService, expanding the to also handle the case of legacy always on. Changes include:

• Change of terminology inside our code base to be more aligned with API. (Prepared / NotPrepared)
• Rework the flow of requesting Vpn Profile
• Make cases more explicit so it is more clear what happens
• Fix regression of INVALID_DNS_SERVERS, so we can show the more explicit error message again.
• Refactor the error messages for when no VPN profile is setup

---

This change is 

[linear]

DROID-1548 IllegalStateException in VpnService.prepare

[Pururun]

Nice work, so much better!

Reviewed 75 of 75 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Rawa)

---

android/lib/resource/src/main/res/values/strings.xml line 109 at r1 (raw file):

    <string name="try_again">Try again</string>
    <string name="blocked_connection">BLOCKED CONNECTION</string>
    <string name="error_state">FAILED TO SECURE CONNECTION</string>

Has this been aligned with design?

---

android/lib/shared/src/main/kotlin/net/mullvad/mullvadvpn/lib/shared/VpnProfileUseCase.kt line 9 at r1 (raw file):

import net.mullvad.mullvadvpn.lib.model.Prepared

class VpnProfileUseCase(private val applicationContext: Context) {

Not really important but almost all other uses cases are using invoke()operator and this does not. I understand why, since we want to wrap context, but maybe this should not be a use case?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/notificationbanner/NotificationData.kt line 109 at r1 (raw file):

        message = error.message().formatWithHtml(),
        statusLevel = StatusLevel.Error,
        action = null,

If I am not mistaken, action is by default?

---

android/lib/resource/src/main/res/values/strings.xml line 172 at r1 (raw file):

        <![CDATA[Unable to start tunnel connection. Please disable Always-on VPN for <b>%s</b> before using Mullvad VPN.]]>
    </string>
    <string name="legacy_always_on_vpn_error_notification_title">Always-on VPN assigned to other app</string>

Should we mention app here? As far as I can tell it is a vpn profile. Could be confusing.

---

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/PrepareError.kt line 8 at r1 (raw file):

sealed interface PrepareError : PrepareResult {
    // Result from VpnService.prepare() being invoked with legacy VPN app has always-on

Nit: Should probably not say legacy vpn app, but rather vpn profile

---

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/PrepareError.kt line 11 at r1 (raw file):

    data object OtherLegacyAlwaysOnVpn : PrepareError

    // Prepare gives intent but there is other always VPN app

Nit: Should at least be "always on VPN" but could maybe be improved further

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/CreateVpnProfile.kt line 8 at r1 (raw file):

import androidx.activity.result.contract.ActivityResultContract

class CreateVpnProfile : ActivityResultContract<Intent, Boolean>() {

I wonder if this could be done even more generic, since all we do is take any intent. So this could be IntentActivityResultContract instead or something.

[Pururun]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/notificationbanner/NotificationData.kt line 109 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

If I am not mistaken, action is by default?

*null

[dlon]

Reviewed 12 of 75 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Rawa)

---

mullvad-daemon/src/lib.rs line 1444 at r1 (raw file):

                // Announce to all clients listening for updates of the
                // currently active access method. The announcement should be
                // made after the firewall policy has been updated, since the

Comment doesn't make sense on Android. protect is called by mullvad-api.

---

talpid-core/src/tunnel_state_machine/connecting_state.rs line 281 at r1 (raw file):

                        #[cfg(target_os = "android")]
                        tunnel::Error::WireguardTunnelMonitoringError(

We could probably clean up this match expression.

[Rawa]

Reviewable status: 73 of 75 files reviewed, 6 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/notificationbanner/NotificationData.kt line 109 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

*null

Correct! Good catch!

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/CreateVpnProfile.kt line 8 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I wonder if this could be done even more generic, since all we do is take any intent. So this could be IntentActivityResultContract instead or something.

I believe the problem is that the parsing of result code might be different for different types of dialogs. For the VPN dialog it just returns a OK / FAIL. but this is not necessarily the case for others and then the contract generics would look different.

---

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/PrepareError.kt line 8 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Nit: Should probably not say legacy vpn app, but rather vpn profile

I've updated the text and clarified it a bit.

---

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/PrepareError.kt line 11 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Nit: Should at least be "always on VPN" but could maybe be improved further

I've updated the text and clarified it a bit.

---

android/lib/resource/src/main/res/values/strings.xml line 109 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Has this been aligned with design?

Yes, I've talked to Matilda.

---

android/lib/resource/src/main/res/values/strings.xml line 172 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Should we mention app here? As far as I can tell it is a vpn profile. Could be confusing.

Let's dicsuss offline

[dlon]

Reviewed 2 of 75 files at r1.
Reviewable status: 73 of 75 files reviewed, 6 unresolved discussions (waiting on @Pururun)

[Rawa]

Reviewable status: 73 of 75 files reviewed, 5 unresolved discussions (waiting on @Pururun)

---

android/lib/shared/src/main/kotlin/net/mullvad/mullvadvpn/lib/shared/VpnProfileUseCase.kt line 9 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Not really important but almost all other uses cases are using invoke()operator and this does not. I understand why, since we want to wrap context, but maybe this should not be a use case?

No you are right, it is wrong. Because it is in the shared module our konsist rules didn't apply. I renamed the use case and fixed the function name 👍 Good catch!

[Pururun]

Reviewed 2 of 2 files at r2, 5 of 5 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/CreateVpnProfile.kt line 8 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

I believe the problem is that the parsing of result code might be different for different types of dialogs. For the VPN dialog it just returns a OK / FAIL. but this is not necessarily the case for others and then the contract generics would look different.

Fair enough :)

---

android/lib/resource/src/main/res/values/strings.xml line 109 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Yes, I've talked to Matilda.

👍

---

android/lib/shared/src/main/kotlin/net/mullvad/mullvadvpn/lib/shared/VpnProfileUseCase.kt line 9 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

No you are right, it is wrong. Because it is in the shared module our konsist rules didn't apply. I renamed the use case and fixed the function name 👍 Good catch!

⭐

[Rawa]

Reviewable status: 73 of 76 files reviewed, 2 unresolved discussions (waiting on @dlon and @Pururun)

---

mullvad-daemon/src/lib.rs line 1444 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Comment doesn't make sense on Android. protect is called by mullvad-api.

Done.

---

talpid-core/src/tunnel_state_machine/connecting_state.rs line 281 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

We could probably clean up this match expression.

Fixed

[Rawa]

Reviewable status: 50 of 76 files reviewed, 2 unresolved discussions (waiting on @dlon and @Pururun)

---

android/lib/resource/src/main/res/values/strings.xml line 172 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Let's dicsuss offline

Resolved

[MarkusPettersson98]

Reviewed 14 of 75 files at r1, 1 of 1 files at r4, 2 of 3 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: 54 of 76 files reviewed, 2 unresolved discussions (waiting on @dlon and @Pururun)

[dlon]

Reviewed 2 of 3 files at r7.
Reviewable status: 54 of 76 files reviewed, 1 unresolved discussion (waiting on @Pururun)

[Pururun]

Reviewed 21 of 21 files at r6, 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

Reviewable status: 50 of 76 files reviewed, all discussions resolved (waiting on @MarkusPettersson98 and @Pururun)

[Pururun]

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