Implement wireguard over shadowsocks

[Pururun]

Enables the use of shadowsocks when using wireguard.

This is blocked by:

• Clean up dialogs #6661 since we want to use the InputDialog added in that PR
• Expose Shadowsocks fd on Android #6760 since we require a new version of shadowsocks rust
• Run VpnService.protect() on Shadowsocks socket before connecting #6781 since we have an issue with reconnect

---

This change is 

[linear]

DROID-1276 Implement Wireguard over shadowsocks

[mullmat]

Design issues found:

• The Shadowsocks custom port has the title WireGuard custom port, this should be Shadowsocks custom port.

---

Otherwise this looks great design wise 🎉 !

[Pururun]

Design issues found:

* The Shadowsocks custom port has the title `WireGuard custom port`, this should be `Shadowsocks custom port`.

Otherwise this looks great design wise 🎉 !

Thanks, fixed :)

[Rawa]

Reviewed 62 of 64 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @Pururun)

---

android/CHANGELOG.md line 27 at r2 (raw file):

### Added
- Add support for predictive back.
- Add wireguard over shadowsocks.

wireguard -> WireGuard
shadowsocks -> Shadowsocks

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectableCell.kt line 110 at r2 (raw file):

                .alpha(
                    if (isSelected && !isEnabled) AlphaDisabled
                    else if (isSelected) AlphaVisible else AlphaInvisible

Maybe a when statement would make this more easy to read?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectObfuscationCell.kt line 51 at r2 (raw file):

@Composable
fun SelectObfuscationCell(
    selectedObfuscation: SelectedObfuscation,

selectedObfuscation is a weird name since it is not necessarily selected at all.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectObfuscationCell.kt line 77 at r2 (raw file):

                    MaterialTheme.colorScheme.selected
                } else {
                    Color.Transparent

Looks a bit weird to have Transparent here, wouldn't it just make more sense to use surfaceContainerLow?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectObfuscationCell.kt line 89 at r2 (raw file):

        if (port != null) {
            VerticalDivider(
                color = Color.Transparent,

If visible if it is Transparent? 😕

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectObfuscationCell.kt line 116 at r2 (raw file):

@Composable
private fun Constraint<Port>?.toSubTitle() =

Do we have to support nullable here? We return null if it is null, so shouldn't be needed?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/TwoRowCell.kt line 29 at r2 (raw file):

fun TwoRowCell(
    titleText: String,
    subtitleText: String?,

I think this is wrong. We should not allow the subtitleText to be null. This allows the "TwoRowCell" to become a "OneRowCell" since the subtitle is omitted if it doesn't exist.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/TwoRowCell.kt line 41 at r2 (raw file):

    isRowEnabled: Boolean = true,
    endPadding: Dp = Dimens.cellEndPadding,
    minHeight: Dp = Dimens.cellHeightTwoRows,

Feels like this composable is starting to blow up with arguments. Is minHeight necessary? Shouldn't it always be the same for all TwoRowCells?

Is the endPadding really needed to pass externally?

Can isRowEnabled be decided by a nullable onCellClicked?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ShadowsocksSettingsScreen.kt line 71 at r2 (raw file):

        navigateToCustomPortDialog =
            dropUnlessResumed {
                val args =

Not sure if formatting/readability is the issue here but, we use this args directly after once, do we need to declare a val for it?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ShadowsocksSettingsScreen.kt line 73 at r2 (raw file):

                val args =
                    CustomPortNavArgs(
                        title =

Do we really need to pass the title, shouldn't the dialog it self be able to produce this?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ShadowsocksSettingsScreen.kt line 113 at r2 (raw file):

                    SelectableCell(
                        title = port.toString(),
                        isSelected = state.port.hasValue(port),

Looks weird to be me with hasValue can't we use ?. or similar operator to compare instead?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 243 at r2 (raw file):

                val args =
                    CustomPortNavArgs(
                        title =

Do we need to pass title as an argument in this case? We should be able to resolve it in the dialog?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 251 at r2 (raw file):

                        allowedPortRanges = state.availablePortRanges,
                    )
                navigator.navigate(CustomPortDestination(args))

Maybe worth considering changing the destination name from CustomPort to WireguardCustomPort? Since we start to expand to multiple options with ports.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/ShadowsocksSettingsState.kt line 9 at r2 (raw file):

data class ShadowsocksSettingsState(
    val port: Constraint<Port> = Constraint.Any,
    val customPort: Port? = null,

How does customPort and port related? There are no restrictions on this type, is it fine to have a port and a custom port?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/PortExtensions.kt line 13 at r2 (raw file):

    }

fun Constraint<Port>.isCustom(presetPorts: List<Int>) =

This feels a bit weird to me, maybe we should consider rethinking our model to avoid this kind of functions. It feels like there should be a more intuitive way to find out if it is custom or not.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ShadowsocksSettingsViewModel.kt line 51 at r2 (raw file):

    init {
        viewModelScope.launch {
            val initialSettings = settingsRepository.settingsUpdates.filterNotNull().first()

I believe firstNotNull() exists.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ShadowsocksSettingsViewModel.kt line 66 at r2 (raw file):

        viewModelScope.launch {
            if (port.isCustom(SHADOWSOCKS_PRESET_PORTS)) {
                customPort.update { port.getOrNull() }

Looks weird to me, we set it no matter if we succeed with the settingsRepository below, shouldn't we only set it if we are successful?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ShadowsocksSettingsViewModel.kt line 75 at r2 (raw file):

    fun resetCustomPort() {
        customPort.update { null }

Same as above.

---

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/SelectedObfuscation.kt line 3 at r2 (raw file):

package net.mullvad.mullvadvpn.lib.model

enum class SelectedObfuscation {

We should consider renaming this class to something else. Since it is not really SelectedObfuscation but rather the options available. It doens't store the data if the option is selected or not.

[Pururun]

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

---

android/CHANGELOG.md line 27 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

wireguard -> WireGuard
shadowsocks -> Shadowsocks

Done.

[Pururun]

Reviewable status: 49 of 65 files reviewed, 16 unresolved discussions (waiting on @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectableCell.kt line 110 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Maybe a when statement would make this more easy to read?

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectObfuscationCell.kt line 77 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Looks a bit weird to have Transparent here, wouldn't it just make more sense to use surfaceContainerLow?

Just to avoid overdraw.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectObfuscationCell.kt line 89 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

If visible if it is Transparent? 😕

Fixed

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectObfuscationCell.kt line 116 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Do we have to support nullable here? We return null if it is null, so shouldn't be needed?

That is true, fixed.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/TwoRowCell.kt line 29 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

I think this is wrong. We should not allow the subtitleText to be null. This allows the "TwoRowCell" to become a "OneRowCell" since the subtitle is omitted if it doesn't exist.

Agree, removed

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/TwoRowCell.kt line 41 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Feels like this composable is starting to blow up with arguments. Is minHeight necessary? Shouldn't it always be the same for all TwoRowCells?

Is the endPadding really needed to pass externally?

Can isRowEnabled be decided by a nullable onCellClicked?

Removed isRowEnabled

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ShadowsocksSettingsScreen.kt line 71 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Not sure if formatting/readability is the issue here but, we use this args directly after once, do we need to declare a val for it?

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ShadowsocksSettingsScreen.kt line 113 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Looks weird to be me with hasValue can't we use ?. or similar operator to compare instead?

Removed hasValue

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 243 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Do we need to pass title as an argument in this case? We should be able to resolve it in the dialog?

Removed args

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/ShadowsocksSettingsState.kt line 9 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

How does customPort and port related? There are no restrictions on this type, is it fine to have a port and a custom port?

Custom port is the value of the custom port cell. Port is the currently selected port which might or might not be the same as the custom port.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/util/PortExtensions.kt line 13 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

This feels a bit weird to me, maybe we should consider rethinking our model to avoid this kind of functions. It feels like there should be a more intuitive way to find out if it is custom or not.

Removed.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ShadowsocksSettingsViewModel.kt line 51 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

I believe firstNotNull() exists.

Did not find any for flow.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ShadowsocksSettingsViewModel.kt line 66 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Looks weird to me, we set it no matter if we succeed with the settingsRepository below, shouldn't we only set it if we are successful?

Moved

[dlon]

Reviewed 4 of 64 files at r1, 1 of 16 files at r4.
Reviewable status: 50 of 65 files reviewed, 16 unresolved discussions (waiting on @Pururun and @Rawa)

[Rawa]

Reviewable status: 8 of 65 files reviewed, 10 unresolved discussions (waiting on @dlon and @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/ShadowsocksSettingsState.kt line 9 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Custom port is the value of the custom port cell. Port is the currently selected port which might or might not be the same as the custom port.

👍 Clarified

[Rawa]

Reviewable status: 8 of 65 files reviewed, 9 unresolved discussions (waiting on @dlon and @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ShadowsocksSettingsViewModel.kt line 51 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Did not find any for flow.

Correct, doesn't exist! Retract!

[Rawa]

Reviewable status: 8 of 65 files reviewed, 9 unresolved discussions (waiting on @dlon and @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ShadowsocksSettingsViewModel.kt line 70 at r5 (raw file):

                    if (
                        port is Constraint.Only &&
                            SHADOWSOCKS_PRESET_PORTS.contains(port.value).not()

Maybe more clear port.value !in SHADOWSOCKS_PRESET_PORTS ?

[Rawa]

Reviewable status: 8 of 65 files reviewed, 9 unresolved discussions (waiting on @dlon and @Pururun)

---

android/CHANGELOG.md line 27 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Done.

nit: Wireguard -> WireGuard

[Rawa]

Reviewed 3 of 16 files at r4, 21 of 54 files at r5.
Reviewable status: 32 of 65 files reviewed, 12 unresolved discussions (waiting on @dlon and @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/TwoRowCell.kt line 66 at r5 (raw file):

        bodyView = bodyView,
        iconView = iconView,
        onCellClicked = onCellClicked ?: {},

Should we clarify base cell as well?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/WireguardConstant.kt line 10 at r5 (raw file):

val SHADOWSOCKS_PRESET_PORTS = emptyList<Port>()
val SHADOWSOCKS_AVAILABLE_PORTS =
    listOf(PortRange(IntRange(0, 65535))) // Currently we consider all ports to be available

PortRange(0..65535)

Maybe we should consider exposing MIN_VALUE and MAX_VALUE from Port.kt to create this one.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VpnSettingsViewModel.kt line 103 at r5 (raw file):

            customPort.update {
                val initialPort = initialSettings.getWireguardPort()
                if (WIREGUARD_PRESET_PORTS.contains(initialPort.getOrNull()).not()) {

initialPort.getOrNull() !in WIREGUARD_PRESET_PORTS

[Pururun]

Reviewable status: 32 of 65 files reviewed, 12 unresolved discussions (waiting on @dlon and @Rawa)

---

android/CHANGELOG.md line 27 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

nit: Wireguard -> WireGuard

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/TwoRowCell.kt line 66 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

Should we clarify base cell as well?

Not sure, there are cases where we want to enable and disable the cell dynamically and not adjust the onclick.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/WireguardConstant.kt line 10 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

PortRange(0..65535)

Maybe we should consider exposing MIN_VALUE and MAX_VALUE from Port.kt to create this one.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ShadowsocksSettingsViewModel.kt line 75 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Same as above.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ShadowsocksSettingsViewModel.kt line 70 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

Maybe more clear port.value !in SHADOWSOCKS_PRESET_PORTS ?

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/VpnSettingsViewModel.kt line 103 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

initialPort.getOrNull() !in WIREGUARD_PRESET_PORTS

Done.

[Rawa]

Reviewed 1 of 54 files at r5.
Reviewable status: 30 of 66 files reviewed, 12 unresolved discussions (waiting on @dlon and @Pururun)

[Rawa]

Reviewed 29 of 54 files at r5, 7 of 7 files at r6, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/WireguardConstant.kt line 11 at r6 (raw file):

val SHADOWSOCKS_AVAILABLE_PORTS =
    // Currently we consider all ports to be available
    listOf(PortRange(IntRange(Port.MIN_VALUE, Port.MAX_VALUE)))

Port Port.MIN_VALUE..Port.MAX_VALUE

[Pururun]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/WireguardConstant.kt line 11 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Port Port.MIN_VALUE..Port.MAX_VALUE

Done.

[Pururun]

Reviewable status: 50 of 66 files reviewed, 6 unresolved discussions (waiting on @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/SelectObfuscationCell.kt line 51 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

selectedObfuscation is a weird name since it is not necessarily selected at all.

Updated naming

---

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/lib/model/SelectedObfuscation.kt line 3 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

We should consider renaming this class to something else. Since it is not really SelectedObfuscation but rather the options available. It doens't store the data if the option is selected or not.

Changed name

[Pururun]

Putting this on hold, since we are partially blocked, see description for more details.

[Rawa]

Reviewed 1 of 1 files at r7, 14 of 15 files at r8, all commit messages.
Reviewable status: 65 of 66 files reviewed, 2 unresolved discussions (waiting on @Pururun)

[Rawa]

Reviewed 1 of 15 files at r8, 37 of 37 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Pururun)

[Pururun]

Reviewable status: 58 of 69 files reviewed, 2 unresolved discussions (waiting on @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ShadowsocksSettingsScreen.kt line 73 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Do we really need to pass the title, shouldn't the dialog it self be able to produce this?

Split into two dialogs

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 251 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Maybe worth considering changing the destination name from CustomPort to WireguardCustomPort? Since we start to expand to multiple options with ports.

Split into two dialogs again.

[Rawa]

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