Merge pull request Tribler#1066 from qstokkink/add_missing_cid_exception

Updated error when community_id is missing
GUI settings should store api_key as bytes for compatibility with previous versions of Tribler

src/tribler-gui/tribler_gui/tests/test_util.py

@@ -1,6 +1,10 @@
+from unittest.mock import MagicMock, patch
 from urllib.parse import unquote_plus
 
-from tribler_gui.utilities import compose_magnetlink, dict_item_is_any_of, quote_plus_unicode, unicode_quoter
+import pytest
+
+from tribler_gui.utilities import compose_magnetlink, dict_item_is_any_of, quote_plus_unicode, set_api_key, \
+    unicode_quoter
 
 
 def test_quoter_char():
@@ -124,3 +128,71 @@ def test_is_dict_has():
     assert dict_item_is_any_of(d, 'k', ['v'])
     assert dict_item_is_any_of(d, 'k', ['v', 'a'])
     assert dict_item_is_any_of(d, 'k', ['a', 'v'])
+
+
+def test_set_api_key():
+    gui_settings = MagicMock()
+
+    # test new api_key not str
+
+    with pytest.raises(ValueError, match="^api_key should be set as str value. Got: bytes$"):
+        set_api_key(gui_settings, b"abcdef")
+    gui_settings.value.assert_not_called()
+    gui_settings.setValue.assert_not_called()
+
+    # test new api_key str
+
+    result = set_api_key(gui_settings, "abcdef")
+
+    assert result == "abcdef"
+    gui_settings.value.assert_not_called()
+    gui_settings.setValue.assert_called_once_with("api_key", b"abcdef")
+
+    with patch("os.urandom") as urandom:
+        hex_mock = MagicMock(return_value="defabc")
+        urandom.return_value = MagicMock(hex=hex_mock)
+
+        # test read bytes api_key value from gui_settings
+
+        gui_settings = MagicMock()
+        gui_settings.value.return_value = b"abcdef"
+
+        result = set_api_key(gui_settings, None)
+
+        assert result == "abcdef"
+        gui_settings.value.assert_called_once_with("api_key", None)
+        hex_mock.assert_not_called()
+        gui_settings.setValue.assert_not_called()
+
+        # test read str api_key value from gui_settings
+
+        gui_settings = MagicMock()
+        gui_settings.value.return_value = "abcdef"
+
+        result = set_api_key(gui_settings, None)
+
+        assert result == "abcdef"
+        gui_settings.value.assert_called_once_with("api_key", None)
+        hex_mock.assert_not_called()
+        gui_settings.setValue.assert_called_once_with("api_key", b"abcdef")
+
+        # test api_key not found in settings, create new random api_key
+
+        gui_settings = MagicMock()
+        gui_settings.value.return_value = None
+
+        result = set_api_key(gui_settings, None)
+
+        assert result == "defabc"
+        gui_settings.value.assert_called_once_with("api_key", None)
+        hex_mock.assert_called_once()
+        gui_settings.setValue.assert_called_once_with("api_key", b"defabc")
+
+        # test unexpected type of api_key from gui_settings
+
+        gui_settings = MagicMock()
+        gui_settings.value.return_value = 123
+
+        match_str = r"^Got unexpected value type of api_key from gui settings \(should be str or bytes\): int$"
+        with pytest.raises(ValueError, match=match_str):
+            set_api_key(gui_settings, None)

src/tribler-gui/tribler_gui/tribler_window.py

@@ -83,6 +83,7 @@
     get_image_path,
     get_ui_file_path,
     is_dir_writable,
+    set_api_key,
     tr,
 )
 from tribler_gui.widgets.channelsmenulistwidget import ChannelsMenuListWidget
@@ -147,11 +148,7 @@ def __init__(
         self.root_state_dir = Path(root_state_dir)
         self.gui_settings = settings
         api_port = api_port or int(get_gui_setting(self.gui_settings, "api_port", DEFAULT_API_PORT))
-        api_key = api_key or get_gui_setting(self.gui_settings, "api_key", hexlify(os.urandom(16)))
-        if isinstance(api_key, bytes):
-            # in QSettings, api_key can be stored as bytes, then we decode it to str
-            api_key = api_key.decode('ascii')
-        self.gui_settings.setValue("api_key", api_key)
+        api_key = set_api_key(self.gui_settings, api_key)
 
         api_port = NetworkUtils().get_first_free_port(start=api_port)
         request_manager.port, request_manager.key = api_port, api_key

src/tribler-gui/tribler_gui/utilities.py

@@ -7,11 +7,11 @@
 import types
 from datetime import datetime, timedelta
 from pathlib import Path
-from typing import Callable
+from typing import Callable, Optional
 from urllib.parse import quote_plus
 from uuid import uuid4
 
-from PyQt5.QtCore import QCoreApplication, QLocale, QTranslator, pyqtSignal
+from PyQt5.QtCore import QCoreApplication, QLocale, QSettings, QTranslator, pyqtSignal
 from PyQt5.QtWidgets import QApplication
 
 import tribler_gui
@@ -222,6 +222,57 @@ def get_gui_setting(gui_settings, value, default, is_bool=False):
     return val
 
 
+def set_api_key(gui_settings: QSettings, api_key: Optional[str] = None) -> str:
+    """
+    Store api_key value to gui_settings if necessary, create a new value if not provided, convert to a correct type.
+
+    If a new api_key value is specified, the method stores the value to gui_settings.
+    If no new api_key value is specified, and gui_settings already contain a value, use the value from gui_settings.
+    If no new api_key value is specified, and gui_settings do not have a value, create a new random value.
+    Store the value to gui_settings if it was not stored already.
+
+    In Tribler, api_key represented as str. In gui_settings it is stored as bytes for compatibility with
+    previous versions of Tribler: if a user installs a new release of Tribler and then rolls back to a previous
+    release, this previous release will crash if the value type of api_key in gui_settings is not bytes.
+    """
+
+
+    should_save = False
+
+    if api_key is not None:
+        if not isinstance(api_key, str):
+            raise ValueError(f'api_key should be set as str value. Got: {type(api_key).__name__}')
+        should_save = True
+
+    else:
+        api_key = get_gui_setting(gui_settings, "api_key", None)
+
+        if api_key is None:
+            api_key = os.urandom(16).hex()
+            should_save = True
+
+        elif isinstance(api_key, bytes):
+            # In Tribler application, api_key is stored as a string. But in gui_settings api_key is stored as bytes,
+            # for compatibility with previous versions of Tribler. Otherwise, a user can get an error if he rolls back
+            # to a previous version of Tribler, that cannot read str values of api_key from GUI settings.
+            api_key = api_key.decode('ascii')
+
+        elif isinstance(api_key, str):
+            # If we read api_keys from gui_settings as str, we need to save it as bytes, to restore
+            # settings compatibility with previous versions of Tribler
+            should_save = True
+
+        elif not isinstance(api_key, str):
+            raise ValueError(f'Got unexpected value type of api_key from gui settings '
+                             f'(should be str or bytes): {type(api_key).__name__}')
+
+    if should_save:
+        api_key_bytes = api_key.encode('ascii')
+        gui_settings.setValue("api_key", api_key_bytes)
+
+    return api_key
+
+
 def is_dir_writable(path):
     """
     Checks if the directory is writable. Creates the directory if one does not exist.