Merge pull request #8033 from drew2a/fix/8014

Refactor proxy settings and add error handling

src/tribler/core/components/libtorrent/download_manager/download_manager.py

@@ -8,11 +8,11 @@
 import os
 import time as timemod
 from asyncio import CancelledError, gather, iscoroutine, shield, sleep, wait_for
-from binascii import unhexlify
 from copy import deepcopy
 from shutil import rmtree
-from typing import Callable, Dict, List, Optional
+from typing import Callable, Dict, List, Optional, Tuple
 
+from binascii import unhexlify
 from ipv8.taskmanager import TaskManager
 
 from tribler.core import notifications
@@ -362,21 +362,35 @@ def get_session(self, hops=0):
 
         return self.ltsessions[hops]
 
-    def set_proxy_settings(self, ltsession, ptype, server=None, auth=None):
+    def set_proxy_settings(self, session, ptype, server: Optional[Tuple[str, str]] = None,
+                           auth: Optional[Tuple[str, str]] = None):
         """
         Apply the proxy settings to a libtorrent session. This mechanism changed significantly in libtorrent 1.1.0.
         """
-        settings = {}
-        settings["proxy_type"] = ptype
-        settings["proxy_hostnames"] = True
-        settings["proxy_peer_connections"] = True
-        if server and server[0] and server[1]:
-            settings["proxy_hostname"] = server[0]
-            settings["proxy_port"] = int(server[1])
-        if auth:
-            settings["proxy_username"] = auth[0]
-            settings["proxy_password"] = auth[1]
-        self.set_session_settings(ltsession, settings)
+        settings = {"proxy_type": ptype, "proxy_hostnames": True, "proxy_peer_connections": True}
+
+        try:
+            if not all(v is not None for v in server):
+                raise ValueError('Host and port should not be None')
+
+            host, port = server
+            port = int(port)
+            settings["proxy_hostname"] = host
+            settings["proxy_port"] = port
+        except (ValueError, TypeError) as e:
+            self._logger.exception(e)
+
+        try:
+            if not all(v is not None for v in auth):
+                raise ValueError('Username and password should not be None')
+
+            username, proxy_password = auth
+            settings["proxy_username"] = username
+            settings["proxy_password"] = proxy_password
+        except (ValueError, TypeError) as e:
+            self._logger.exception(e)
+
+        self.set_session_settings(session, settings)
 
     def set_upload_rate_limit(self, rate, hops=None):
         # Rate conversion due to the fact that we had a different system with Swift

src/tribler/core/components/libtorrent/tests/test_download_manager.py

@@ -1,5 +1,4 @@
 import asyncio
-import functools
 import itertools
 from asyncio import Future
 from unittest.mock import AsyncMock, MagicMock, Mock
@@ -347,28 +346,15 @@ def test_set_proxy_settings(fake_dlmgr):
     """
     Test setting the proxy settings
     """
-
-    def on_proxy_set(settings):
-        assert settings
-        assert settings.hostname == 'a'
-        assert settings.port == 1234
-        assert settings.username == 'abc'
-        assert settings.password == 'def'
-
-    def on_set_settings(settings):
-        assert settings
-        assert settings['proxy_hostname'] == 'a'
-        assert settings['proxy_port'] == 1234
-        assert settings['proxy_username'] == 'abc'
-        assert settings['proxy_password'] == 'def'
-        assert settings['proxy_peer_connections']
-        assert settings['proxy_hostnames']
-
-    mock_lt_session = MagicMock()
-    mock_lt_session.get_settings = dict
-    mock_lt_session.set_settings = on_set_settings
-    mock_lt_session.set_proxy = on_proxy_set  # Libtorrent < 1.1.0 uses set_proxy to set proxy settings
-    fake_dlmgr.set_proxy_settings(mock_lt_session, 0, ('a', "1234"), ('abc', 'def'))
+    session = Mock()
+    fake_dlmgr.set_proxy_settings(session, 0, ('a', "1234"), ('abc', 'def'))
+    settings, = session.method_calls[-1].args
+    assert settings['proxy_hostname'] == 'a'
+    assert settings['proxy_port'] == 1234
+    assert settings['proxy_username'] == 'abc'
+    assert settings['proxy_password'] == 'def'
+    assert settings['proxy_peer_connections']
+    assert settings['proxy_hostnames']
 
 
 def test_payout_on_disconnect(fake_dlmgr):
@@ -575,3 +561,53 @@ def test_update_trackers_list_append(fake_dlmgr) -> None:
     actual_trackers = set(itertools.chain.from_iterable(tracker_list))
     assert actual_trackers == {fake_tracker1, fake_tracker2}
 
+
+def test_set_proxy_settings_invalid_port(fake_dlmgr):
+    # Test setting the proxy settings for an invalid port number. In this case port and host should not be set.
+    session = Mock()
+    proxy_type = 2
+
+    fake_dlmgr.set_proxy_settings(session, proxy_type, ('host name', 'invalid port'))
+
+    settings, = session.method_calls[-1].args
+    assert 'proxy_port' not in settings
+    assert 'proxy_hostname' not in settings
+    assert settings['proxy_type'] == proxy_type
+
+
+def test_set_proxy_defaults(fake_dlmgr):
+    # Test setting the proxy settings with default values
+    session = Mock()
+    proxy_type = 2
+
+    fake_dlmgr.set_proxy_settings(session, proxy_type)
+    settings, = session.method_calls[-1].args
+    assert 'proxy_port' not in settings
+    assert 'proxy_hostname' not in settings
+    assert settings['proxy_type'] == proxy_type
+
+
+def test_set_proxy_corner_case(fake_dlmgr):
+    # Test setting the proxy settings with None values
+    session = Mock()
+
+    fake_dlmgr.set_proxy_settings(session, 2, (None, None))
+    settings, = session.method_calls[-1].args
+    assert settings['proxy_type'] == 2
+    assert 'proxy_port' not in settings
+
+    fake_dlmgr.set_proxy_settings(session, 3, (None,))
+    settings, = session.method_calls[-1].args
+    assert settings['proxy_type'] == 3
+    assert 'proxy_port' not in settings
+
+    fake_dlmgr.set_proxy_settings(session, 3, (None, 123))
+    settings, = session.method_calls[-1].args
+    assert settings['proxy_type'] == 3
+    assert 'proxy_port' not in settings
+
+    fake_dlmgr.set_proxy_settings(session, 3, (None, 123), ('name', None))
+    settings, = session.method_calls[-1].args
+    assert settings['proxy_type'] == 3
+    assert 'proxy_port' not in settings
+    assert 'proxy_username' not in settings