Correctly handle removal & disabling of access method

[MarkusPettersson98]

This PR aims to fix a few holes in the logic of enabling/disabling/removing access methods in different edge cases.

• If the access method currently in-use is disabled → Switch to the next available access method
• If the last enabled access method is removed → Enable the direct access method and switch to it
• If the last enabled access method is currently in-use and removed → Enable the direct access method and switch to it

This logic can somewhat be simplified by restructuring the access methods part of the settings datastructure to always include the predefined Direct and Mullvad Bridges access methods. Piggybacking on the settings migration started by the custom OpenVPN SOCKS5 bridge PR, this PR alters the layout of the serialized access method settings accordingly.

This PR also fixes DES-547

---

This change is 

[linear]

DES-561 Correctly handle removal & disabling of access method

[dlon]

Reviewed 1 of 3 files at r1, 2 of 4 files at r2, 1 of 2 files at r3, 10 of 10 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)

---

mullvad-daemon/src/access_method.rs line 181 at r4 (raw file):

                *access_method = access_method_update.clone();
            }
            ensure_direct_is_enabled(settings);

Although this is fine, I think it would be nicer if it it were impossible for the collection itself to end up in an "invalid" state (e.g. no enabled methods) instead of expecting the client to make sure that this is the case (discussed offline).

---

mullvad-management-interface/src/types/conversions/access_method.rs line 14 at r4 (raw file):

                mullvad_bridges: Some(settings.mullvad_bridges.into()),
                access_method_settings: settings
                    .user_defined

Nit: Would be slightly nicer to stick to either "custom" or "user-defined" (but only use one of them).

[linear]

DES-547 Separate built-in access methods settings from user-specified dito

[MarkusPettersson98]

@raksooo Please have a look at the modifications made in gui/ 😊

[raksooo]

Reviewed 6 of 8 files at r6.
Reviewable status: 11 of 18 files reviewed, 3 unresolved discussions (waiting on @dlon and @MarkusPettersson98)

---

gui/src/shared/daemon-rpc-types.ts line 532 at r6 (raw file):

export type ApiAccessMethodSettings = {
  direct: AccessMethodSetting;
  mullvad_bridges: AccessMethodSetting;

I thought the linter would complain about this, I'll have to look into it. We use camelCase for variables and object properties.

[MarkusPettersson98]

Reviewable status: 5 of 18 files reviewed, 1 unresolved discussion (waiting on @dlon and @raksooo)

---

mullvad-daemon/src/access_method.rs line 181 at r4 (raw file):

Previously, dlon (David Lönnhager) wrote…

Although this is fine, I think it would be nicer if it it were impossible for the collection itself to end up in an "invalid" state (e.g. no enabled methods) instead of expecting the client to make sure that this is the case (discussed offline).

Done

---

mullvad-management-interface/src/types/conversions/access_method.rs line 14 at r4 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: Would be slightly nicer to stick to either "custom" or "user-defined" (but only use one of them).

Done

[MarkusPettersson98]

Reviewable status: 5 of 21 files reviewed, 1 unresolved discussion (waiting on @dlon and @raksooo)

---

gui/src/shared/daemon-rpc-types.ts line 532 at r6 (raw file):

Previously, raksooo (Oskar Nyberg) wrote…

I thought the linter would complain about this, I'll have to look into it. We use camelCase for variables and object properties.

Done

Added a rule to cover this to the ESLint config

[dlon]

Reviewed 1 of 6 files at r5, 7 of 13 files at r8, all commit messages.
Reviewable status: 12 of 21 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98 and @raksooo)

---

mullvad-daemon/src/access_method.rs line 181 at r4 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Done

Awesome :)

---

mullvad-types/src/access_method.rs line 35 at r9 (raw file):

    /// This function will return an error if a built-in API access is about to
    /// be removed.
    pub fn remove(&mut self, api_access_method: &Id) -> Result<(), Error> {

Do we need to check if the enabled collection is empty here as well? In case the only active (non-builtin) method is deleted.

[MarkusPettersson98]

Reviewable status: 11 of 21 files reviewed, 2 unresolved discussions (waiting on @dlon and @raksooo)

---

mullvad-types/src/access_method.rs line 35 at r9 (raw file):

Previously, dlon (David Lönnhager) wrote…

Do we need to check if the enabled collection is empty here as well? In case the only active (non-builtin) method is deleted.

Hmm, your are right. We used to have this check when a custom access was deleted. Re-added!

[raksooo]

Reviewed 1 of 13 files at r8, 8 of 8 files at r9.
Reviewable status: 20 of 21 files reviewed, 1 unresolved discussion (waiting on @dlon)

---

gui/src/shared/daemon-rpc-types.ts line 532 at r6 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Done

Added a rule to cover this to the ESLint config

Great! 👍

[dlon]

Reviewed 4 of 5 files at r11, all commit messages.
Reviewable status: 20 of 21 files reviewed, all discussions resolved (waiting on @raksooo)

---

mullvad-daemon/src/api.rs line 433 at r11 (raw file):

        // The action would still be valid, since modulo will make sure we stay
        // within bounds.
        self.index %= self.access_method_settings.cardinality();

This might be unnecessary given that we always use modulo before using the index?

[MarkusPettersson98]

Reviewable status: 20 of 21 files reviewed, all discussions resolved (waiting on @raksooo)

---

mullvad-daemon/src/api.rs line 433 at r11 (raw file):

Previously, dlon (David Lönnhager) wrote…

This might be unnecessary given that we always use modulo before using the index?

It might be unnecessary, given that we rotate the access method when removing access methods which is the case I'm guarding for here. Removing this piece of code since I don't think it accomplishes anything 😊

[dlon]

Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: 20 of 21 files reviewed, all discussions resolved (waiting on @raksooo)

[dlon]

Reviewed 1 of 5 files at r11, 1 of 1 files at r13, 1 of 1 files at r15, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved