Code cleanup

- Get rid of extraneous calls to `clone`
- Address nit: combine similar match arms into a single match arm
- Fix `clippy` lint "unused `async` for function with no await statements"
- Fix protobuf field numbers should start from 1
  - This was not the case for `Socks5Local` & `Shadowsocks`
- Refactor code for opening proxy connections
  - Cut down on duplicated code for setting up a proxied connection in
  `mullvad-api`. The difference between different connection modes is
  how they wrap the underlying `TCP` stream.
- Remove `enable_access_method` & `disable_access_method` from RPC-client

7 files changed, 122 insertions, 163 deletions

diff --git a/mullvad-api/src/https_client_with_sni.rs b/mullvad-api/src/https_client_with_sni.rs
index cc7557a8d2..47b6923d37 100644
--- a/mullvad-api/src/https_client_with_sni.rs
+++ b/mullvad-api/src/https_client_with_sni.rs
@@ -36,6 +36,7 @@ use std::{
 use talpid_types::ErrorExt;
 
 use tokio::{
+    io::{AsyncRead, AsyncWrite},
     net::{TcpSocket, TcpStream},
     time::timeout,
 };
@@ -81,37 +82,59 @@ enum InnerConnectionMode {
 
 impl InnerConnectionMode {
     async fn connect(
-        &self,
+        self,
         hostname: &str,
         addr: &SocketAddr,
         #[cfg(target_os = "android")] socket_bypass_tx: Option<mpsc::Sender<SocketBypassRequest>>,
     ) -> Result<ApiConnection, std::io::Error> {
-        use InnerConnectionMode::*;
         match self {
-            Direct => {
-                Self::handle_direct_connection(
-                    addr,
+            // Set up a TCP-socket connection.
+            InnerConnectionMode::Direct => {
+                let first_hop = *addr;
+                let make_proxy_stream = |tcp_stream| async { Ok(tcp_stream) };
+                Self::connect_proxied(
+                    first_hop,
                     hostname,
+                    make_proxy_stream,
                     #[cfg(target_os = "android")]
                     socket_bypass_tx,
                 )
                 .await
             }
-            Shadowsocks(config) => {
-                Self::handle_shadowsocks_connection(
-                    config.clone(),
-                    addr,
+            // Set up a Shadowsocks-connection.
+            InnerConnectionMode::Shadowsocks(shadowsocks) => {
+                let first_hop = shadowsocks.params.peer;
+                let make_proxy_stream = |tcp_stream| async {
+                    Ok(ProxyClientStream::from_stream(
+                        shadowsocks.proxy_context,
+                        tcp_stream,
+                        &ServerConfig::from(shadowsocks.params),
+                        *addr,
+                    ))
+                };
+                Self::connect_proxied(
+                    first_hop,
                     hostname,
+                    make_proxy_stream,
                     #[cfg(target_os = "android")]
                     socket_bypass_tx,
                 )
                 .await
             }
-            Socks5(proxy_config) => {
-                Self::handle_socks_connection(
-                    proxy_config.clone(),
-                    addr,
+            // Set up a SOCKS5-connection.
+            InnerConnectionMode::Socks5(socks) => {
+                let first_hop = socks.peer;
+                let make_proxy_stream = |tcp_stream| async {
+                    tokio_socks::tcp::Socks5Stream::connect_with_socket(tcp_stream, addr)
+                        .await
+                        .map_err(|error| {
+                            io::Error::new(io::ErrorKind::Other, format!("SOCKS error: {error}"))
+                        })
+                };
+                Self::connect_proxied(
+                    first_hop,
                     hostname,
+                    make_proxy_stream,
                     #[cfg(target_os = "android")]
                     socket_bypass_tx,
                 )
@@ -120,74 +143,37 @@ impl InnerConnectionMode {
         }
     }
 
-    /// Set up a TCP-socket connection.
-    async fn handle_direct_connection(
-        addr: &SocketAddr,
-        hostname: &str,
-        #[cfg(target_os = "android")] socket_bypass_tx: Option<mpsc::Sender<SocketBypassRequest>>,
-    ) -> Result<ApiConnection, io::Error> {
-        let socket = HttpsConnectorWithSni::open_socket(
-            *addr,
-            #[cfg(target_os = "android")]
-            socket_bypass_tx,
-        )
-        .await?;
-        #[cfg(feature = "api-override")]
-        if API.disable_tls {
-            return Ok(ApiConnection::new(Box::new(socket)));
-        }
-
-        let tls_stream = TlsStream::connect_https(socket, hostname).await?;
-        Ok(ApiConnection::new(Box::new(tls_stream)))
-    }
-
-    /// Set up a shadowsocks-socket connection.
-    async fn handle_shadowsocks_connection(
-        shadowsocks: ShadowsocksConfig,
-        addr: &SocketAddr,
+    /// Create an [`ApiConnection`] from a [`TcpStream`].
+    ///
+    /// The `make_proxy_stream` closure receives a [`TcpStream`] and produces a
+    /// stream which can send to and receive data from some server using any
+    /// proxy protocol. The only restriction is that this stream must implement
+    /// [`tokio::io::AsyncRead`] and [`tokio::io::AsyncWrite`], as well as
+    /// [`Unpin`] and [`Send`].
+    ///
+    /// If a direct connection is to be established (i.e. the stream will not be
+    /// using any proxy protocol) `make_proxy_stream` may return the
+    /// [`TcpStream`] itself. See for example how a connection is established
+    /// from connection mode [`InnerConnectionMode::Direct`].
+    async fn connect_proxied<ProxyFactory, ProxyFuture, Proxy>(
+        first_hop: SocketAddr,
         hostname: &str,
+        make_proxy_stream: ProxyFactory,
         #[cfg(target_os = "android")] socket_bypass_tx: Option<mpsc::Sender<SocketBypassRequest>>,
-    ) -> Result<ApiConnection, io::Error> {
+    ) -> Result<ApiConnection, io::Error>
+    where
+        ProxyFactory: FnOnce(TcpStream) -> ProxyFuture,
+        ProxyFuture: Future<Output = io::Result<Proxy>>,
+        Proxy: AsyncRead + AsyncWrite + Unpin + Send + 'static,
+    {
         let socket = HttpsConnectorWithSni::open_socket(
-            shadowsocks.params.peer,
+            first_hop,
             #[cfg(target_os = "android")]
             socket_bypass_tx,
         )
         .await?;
-        let proxy = ProxyClientStream::from_stream(
-            shadowsocks.proxy_context,
-            socket,
-            &ServerConfig::from(shadowsocks.params),
-            *addr,
-        );
-
-        #[cfg(feature = "api-override")]
-        if API.disable_tls {
-            return Ok(ApiConnection::new(Box::new(ConnectionDecorator(proxy))));
-        }
 
-        let tls_stream = TlsStream::connect_https(proxy, hostname).await?;
-        Ok(ApiConnection::new(Box::new(tls_stream)))
-    }
-
-    /// Set up a SOCKS5-socket connection.
-    async fn handle_socks_connection(
-        proxy_config: SocksConfig,
-        addr: &SocketAddr,
-        hostname: &str,
-        #[cfg(target_os = "android")] socket_bypass_tx: Option<mpsc::Sender<SocketBypassRequest>>,
-    ) -> Result<ApiConnection, io::Error> {
-        let socket = HttpsConnectorWithSni::open_socket(
-            proxy_config.peer,
-            #[cfg(target_os = "android")]
-            socket_bypass_tx.clone(),
-        )
-        .await?;
-        let proxy = tokio_socks::tcp::Socks5Stream::connect_with_socket(socket, addr)
-            .await
-            .map_err(|error| {
-                io::Error::new(io::ErrorKind::Other, format!("SOCKS error: {error}"))
-            })?;
+        let proxy = make_proxy_stream(socket).await?;
 
         #[cfg(feature = "api-override")]
         if API.disable_tls {
diff --git a/mullvad-cli/src/cmds/api_access.rs b/mullvad-cli/src/cmds/api_access.rs
index 4c1594b1fe..a3b17aca0b 100644
--- a/mullvad-cli/src/cmds/api_access.rs
+++ b/mullvad-cli/src/cmds/api_access.rs
@@ -77,7 +77,7 @@ impl ApiAccess {
     /// Add a custom API access method.
     async fn add(cmd: AddCustomCommands) -> Result<()> {
         let mut rpc = MullvadProxyClient::new().await?;
-        let name = cmd.name();
+        let name = cmd.name().to_string();
         let enabled = cmd.enabled();
         let access_method = AccessMethod::try_from(cmd)?;
         rpc.add_access_method(name, enabled, access_method).await?;
@@ -143,16 +143,18 @@ impl ApiAccess {
     /// Enable a custom API access method.
     async fn enable(item: SelectItem) -> Result<()> {
         let mut rpc = MullvadProxyClient::new().await?;
-        let access_method = Self::get_access_method(&mut rpc, &item).await?;
-        rpc.enable_access_method(access_method.get_id()).await?;
+        let mut access_method = Self::get_access_method(&mut rpc, &item).await?;
+        access_method.enable();
+        rpc.update_access_method(access_method).await?;
         Ok(())
     }
 
     /// Disable a custom API access method.
     async fn disable(item: SelectItem) -> Result<()> {
         let mut rpc = MullvadProxyClient::new().await?;
-        let access_method = Self::get_access_method(&mut rpc, &item).await?;
-        rpc.disable_access_method(access_method.get_id()).await?;
+        let mut access_method = Self::get_access_method(&mut rpc, &item).await?;
+        access_method.disable();
+        rpc.update_access_method(access_method).await?;
         Ok(())
     }
 
@@ -262,24 +264,19 @@ pub enum AddSocks5Commands {
 }
 
 impl AddCustomCommands {
-    fn name(&self) -> String {
+    fn name(&self) -> &str {
         match self {
-            AddCustomCommands::Shadowsocks { name, .. } => name,
-            AddCustomCommands::Socks5(socks) => match socks {
-                AddSocks5Commands::Remote { name, .. } => name,
-                AddSocks5Commands::Local { name, .. } => name,
-            },
+            AddCustomCommands::Shadowsocks { name, .. }
+            | AddCustomCommands::Socks5(AddSocks5Commands::Remote { name, .. })
+            | AddCustomCommands::Socks5(AddSocks5Commands::Local { name, .. }) => name,
         }
-        .clone()
     }
 
     fn enabled(&self) -> bool {
         match self {
-            AddCustomCommands::Shadowsocks { disabled, .. } => !disabled,
-            AddCustomCommands::Socks5(socks) => match socks {
-                AddSocks5Commands::Remote { disabled, .. } => !disabled,
-                AddSocks5Commands::Local { disabled, .. } => !disabled,
-            },
+            AddCustomCommands::Shadowsocks { disabled, .. }
+            | AddCustomCommands::Socks5(AddSocks5Commands::Remote { disabled, .. })
+            | AddCustomCommands::Socks5(AddSocks5Commands::Local { disabled, .. }) => !disabled,
         }
     }
 }
diff --git a/mullvad-daemon/src/access_method.rs b/mullvad-daemon/src/access_method.rs
index 73fd35fcbd..ea096cf5ba 100644
--- a/mullvad-daemon/src/access_method.rs
+++ b/mullvad-daemon/src/access_method.rs
@@ -102,7 +102,7 @@ where
 
     /// Return the [`AccessMethodSetting`] which is currently used to access the
     /// Mullvad API.
-    pub async fn get_current_access_method(&mut self) -> Result<AccessMethodSetting, Error> {
+    pub fn get_current_access_method(&mut self) -> Result<AccessMethodSetting, Error> {
         let connections_modes = self.connection_modes.lock().unwrap();
         Ok(connections_modes.peek())
     }
diff --git a/mullvad-daemon/src/api.rs b/mullvad-daemon/src/api.rs
index d44bdd0849..c548f0a293 100644
--- a/mullvad-daemon/src/api.rs
+++ b/mullvad-daemon/src/api.rs
@@ -10,7 +10,7 @@ use mullvad_api::{
     ApiEndpointUpdateCallback,
 };
 use mullvad_relay_selector::RelaySelector;
-use mullvad_types::access_method::AccessMethodSetting;
+use mullvad_types::access_method::{AccessMethod, AccessMethodSetting, BuiltInAccessMethod};
 use std::{
     net::SocketAddr,
     path::PathBuf,
@@ -106,10 +106,13 @@ impl ApiConnectionModeProvider {
         log::debug!("Rotating Access mode!");
         let access_method = {
             let mut access_methods_picker = self.connection_modes.lock().unwrap();
-            access_methods_picker.next()
+            access_methods_picker
+                .next()
+                .map(|access_method_setting| access_method_setting.access_method)
+                .unwrap_or(AccessMethod::from(BuiltInAccessMethod::Direct))
         };
 
-        let connection_mode = self.from(access_method.as_ref());
+        let connection_mode = self.from(access_method);
         log::info!("New API connection mode selected: {}", connection_mode);
         connection_mode
     }
@@ -118,45 +121,40 @@ impl ApiConnectionModeProvider {
     /// [`ApiConnectionMode`]s require extra logic/data from
     /// [`ApiConnectionModeProvider`] the standard [`std::convert::From`] trait
     /// can not be implemented.
-    fn from(&mut self, access_method: Option<&AccessMethodSetting>) -> ApiConnectionMode {
-        use mullvad_types::access_method::{self, AccessMethod, BuiltInAccessMethod};
+    fn from(&mut self, access_method: AccessMethod) -> ApiConnectionMode {
+        use mullvad_types::access_method;
         match access_method {
-            None => ApiConnectionMode::Direct,
-            Some(access_method) => match &access_method.access_method {
-                AccessMethod::BuiltIn(access_method) => match access_method {
-                    BuiltInAccessMethod::Direct => ApiConnectionMode::Direct,
-                    BuiltInAccessMethod::Bridge => self
-                        .relay_selector
-                        .get_bridge_forced()
-                        .and_then(|settings| match settings {
-                            ProxySettings::Shadowsocks(ss_settings) => {
-                                let ss_settings: access_method::Shadowsocks =
-                                    access_method::Shadowsocks::new(
-                                        ss_settings.peer,
-                                        ss_settings.cipher,
-                                        ss_settings.password,
-                                    );
-                                Some(ApiConnectionMode::Proxied(ProxyConfig::Shadowsocks(
-                                    ss_settings,
-                                )))
-                            }
-                            _ => {
-                                log::error!("Received unexpected proxy settings type");
-                                None
-                            }
-                        })
-                        .unwrap_or(ApiConnectionMode::Direct),
-                },
-                AccessMethod::Custom(access_method) => match &access_method {
-                    access_method::CustomAccessMethod::Shadowsocks(shadowsocks_config) => {
-                        ApiConnectionMode::Proxied(ProxyConfig::Shadowsocks(
-                            shadowsocks_config.clone(),
-                        ))
-                    }
-                    access_method::CustomAccessMethod::Socks5(socks_config) => {
-                        ApiConnectionMode::Proxied(ProxyConfig::Socks(socks_config.clone()))
-                    }
-                },
+            AccessMethod::BuiltIn(access_method) => match access_method {
+                BuiltInAccessMethod::Direct => ApiConnectionMode::Direct,
+                BuiltInAccessMethod::Bridge => self
+                    .relay_selector
+                    .get_bridge_forced()
+                    .and_then(|settings| match settings {
+                        ProxySettings::Shadowsocks(ss_settings) => {
+                            let ss_settings: access_method::Shadowsocks =
+                                access_method::Shadowsocks::new(
+                                    ss_settings.peer,
+                                    ss_settings.cipher,
+                                    ss_settings.password,
+                                );
+                            Some(ApiConnectionMode::Proxied(ProxyConfig::Shadowsocks(
+                                ss_settings,
+                            )))
+                        }
+                        _ => {
+                            log::error!("Received unexpected proxy settings type");
+                            None
+                        }
+                    })
+                    .unwrap_or(ApiConnectionMode::Direct),
+            },
+            AccessMethod::Custom(access_method) => match access_method {
+                access_method::CustomAccessMethod::Shadowsocks(shadowsocks_config) => {
+                    ApiConnectionMode::Proxied(ProxyConfig::Shadowsocks(shadowsocks_config))
+                }
+                access_method::CustomAccessMethod::Socks5(socks_config) => {
+                    ApiConnectionMode::Proxied(ProxyConfig::Socks(socks_config))
+                }
             },
         }
     }
diff --git a/mullvad-daemon/src/lib.rs b/mullvad-daemon/src/lib.rs
index 0a00639c09..3def0855c2 100644
--- a/mullvad-daemon/src/lib.rs
+++ b/mullvad-daemon/src/lib.rs
@@ -1076,7 +1076,7 @@ where
             }
             RemoveApiAccessMethod(tx, method) => self.on_remove_api_access_method(tx, method).await,
             UpdateApiAccessMethod(tx, method) => self.on_update_api_access_method(tx, method).await,
-            GetCurrentAccessMethod(tx) => self.on_get_current_api_access_method(tx).await,
+            GetCurrentAccessMethod(tx) => self.on_get_current_api_access_method(tx),
             SetApiAccessMethod(tx, method) => self.on_set_api_access_method(tx, method),
             GetApiAddresses(tx) => self.on_get_api_addresses(tx).await,
             IsPerformingPostUpgrade(tx) => self.on_is_performing_post_upgrade(tx),
@@ -2307,13 +2307,9 @@ where
         Self::oneshot_send(tx, result, "update_api_access_method response");
     }
 
-    async fn on_get_current_api_access_method(
-        &mut self,
-        tx: ResponseTx<AccessMethodSetting, Error>,
-    ) {
+    fn on_get_current_api_access_method(&mut self, tx: ResponseTx<AccessMethodSetting, Error>) {
         let result = self
             .get_current_access_method()
-            .await
             .map_err(Error::AccessMethodError);
         Self::oneshot_send(tx, result, "get_current_api_access_method response");
     }
diff --git a/mullvad-management-interface/proto/management_interface.proto b/mullvad-management-interface/proto/management_interface.proto
index 412358c9c5..c9b9bc5138 100644
--- a/mullvad-management-interface/proto/management_interface.proto
+++ b/mullvad-management-interface/proto/management_interface.proto
@@ -338,19 +338,19 @@ message AccessMethod {
   message Direct {}
   message Bridges {}
   message Socks5Local {
-    string ip = 4;
-    uint32 port = 5;
-    uint32 local_port = 6;
+    string ip = 1;
+    uint32 port = 2;
+    uint32 local_port = 3;
   }
   message Socks5Remote {
     string ip = 2;
     uint32 port = 3;
   }
   message Shadowsocks {
-    string ip = 2;
-    uint32 port = 3;
-    string password = 4;
-    string cipher = 5;
+    string ip = 1;
+    uint32 port = 2;
+    string password = 3;
+    string cipher = 4;
   }
   oneof access_method {
     Direct direct = 1;
diff --git a/mullvad-management-interface/src/client.rs b/mullvad-management-interface/src/client.rs
index 9e1b3772db..a4d56dd445 100644
--- a/mullvad-management-interface/src/client.rs
+++ b/mullvad-management-interface/src/client.rs
@@ -519,24 +519,6 @@ impl MullvadProxyClient {
             .map(drop)
     }
 
-    pub async fn enable_access_method(
-        &mut self,
-        api_access_method_id: access_method::Id,
-    ) -> Result<()> {
-        let mut new_api_access_method = self.get_api_access_method(&api_access_method_id).await?;
-        new_api_access_method.enable();
-        self.update_access_method(new_api_access_method).await
-    }
-
-    pub async fn disable_access_method(
-        &mut self,
-        api_access_method_id: access_method::Id,
-    ) -> Result<()> {
-        let mut new_api_access_method = self.get_api_access_method(&api_access_method_id).await?;
-        new_api_access_method.disable();
-        self.update_access_method(new_api_access_method).await
-    }
-
     pub async fn remove_access_method(
         &mut self,
         api_access_method: access_method::Id,