change the behavior - insecure is default

also add tests

Author: vladak

adafruit_minimqtt/adafruit_minimqtt.py

@@ -171,7 +171,7 @@ def __init__(
         username=None,
         password=None,
         client_id=None,
-        is_ssl=True,
+        is_ssl=None,
         keep_alive=60,
         recv_timeout=10,
         socket_pool=None,
@@ -220,9 +220,14 @@ def __init__(
         ):  # [MQTT-3.1.3.5]
             raise MMQTTException("Password length is too large.")
 
+        # The connection will be insecure unless is_ssl is set to True.
+        # If the port is not specified, the security will be set based on the is_ssl parameter.
+        # If the port is specified, the is_ssl parameter will be honored.
         self.port = MQTT_TCP_PORT
-        if is_ssl:
-            self._is_ssl = True
+        if is_ssl is None:
+            is_ssl = False
+        self._is_ssl = is_ssl
+        if self._is_ssl:
             self.port = MQTT_TLS_PORT
         if port:
             self.port = port

tests/test_port_ssl.py

@@ -0,0 +1,132 @@
+# SPDX-FileCopyrightText: 2023 Vladimír Kotal
+#
+# SPDX-License-Identifier: Unlicense
+
+"""tests that verify the connect behavior w.r.t. port number and TLS"""
+
+import socket
+import ssl
+from unittest import TestCase, main
+from unittest.mock import Mock, call, patch
+
+import adafruit_minimqtt.adafruit_minimqtt as MQTT
+
+
+class PortSslSetup(TestCase):
+    """This class contains tests that verify how host/port and TLS is set for connect().
+    These tests assume that there is no MQTT broker running on the hosts/ports they connect to.
+    """
+
+    def test_default_port(self) -> None:
+        """verify default port value and that TLS is not used"""
+        host = "127.0.0.1"
+        port = 1883
+
+        with patch.object(socket.socket, "connect") as connect_mock:
+            ssl_context = ssl.create_default_context()
+            mqtt_client = MQTT.MQTT(
+                broker=host,
+                socket_pool=socket,
+                ssl_context=ssl_context,
+                connect_retries=1,
+            )
+
+            ssl_mock = Mock()
+            ssl_context.wrap_socket = ssl_mock
+
+            with self.assertRaises(OSError):
+                expected_port = port
+                mqtt_client.connect()
+
+            ssl_mock.assert_not_called()
+            connect_mock.assert_called()
+            # Assuming the repeated calls will have the same arguments.
+            connect_mock.assert_has_calls(
+                [call((host, expected_port))]
+            )
+
+    def test_connect_override(self):
+        """Test that connect() can override host and port."""
+        host = "127.0.0.1"
+        port = 1883
+
+        with patch.object(socket.socket, "connect") as connect_mock:
+            connect_mock.side_effect = OSError("artificial error")
+            mqtt_client = MQTT.MQTT(
+                broker=host,
+                port=port,
+                socket_pool=socket,
+                connect_retries=1,
+            )
+
+            with self.assertRaises(OSError):
+                expected_host = "127.0.0.2"
+                expected_port = 1884
+                self.assertNotEqual(
+                    expected_port, port, "port override should differ"
+                )
+                self.assertNotEqual(
+                    expected_host, host, "host override should differ"
+                )
+                mqtt_client.connect(host=expected_host, port=expected_port)
+
+            connect_mock.assert_called()
+            # Assuming the repeated calls will have the same arguments.
+            connect_mock.assert_has_calls(
+                [call((expected_host, expected_port))]
+            )
+
+    def test_tls_port(self) -> None:
+        """verify that when is_ssl=True is set, the default port is 8883
+        and the socket is TLS wrapped. Also test that the TLS port can be overridden."""
+        host = "127.0.0.1"
+
+        for port in [None, 8884]:
+            if port is None:
+                expected_port = 8883
+            else:
+                expected_port = port
+            with self.subTest():
+                ssl_mock = Mock()
+                mqtt_client = MQTT.MQTT(
+                    broker=host,
+                    port=port,
+                    socket_pool=socket,
+                    is_ssl=True,
+                    ssl_context=ssl_mock,
+                    connect_retries=1,
+                )
+
+                socket_mock = Mock()
+                connect_mock = Mock(side_effect=OSError)
+                socket_mock.connect = connect_mock
+                ssl_mock.wrap_socket = Mock(return_value=socket_mock)
+
+                with self.assertRaises(RuntimeError):
+                    mqtt_client.connect()
+
+                ssl_mock.wrap_socket.assert_called()
+
+                connect_mock.assert_called()
+                # Assuming the repeated calls will have the same arguments.
+                connect_mock.assert_has_calls([call((host, expected_port))])
+
+    def test_tls_without_ssl_context(self) -> None:
+        """verify that when is_ssl=True is set, the code will check that ssl_context is not None"""
+        host = "127.0.0.1"
+
+        mqtt_client = MQTT.MQTT(
+            broker=host,
+            socket_pool=socket,
+            is_ssl=True,
+            ssl_context=None,
+            connect_retries=1,
+        )
+
+        with self.assertRaises(RuntimeError) as context:
+            mqtt_client.connect()
+            self.assertTrue("ssl_context must be set" in str(context))
+
+
+if __name__ == "__main__":
+    main()

tox.ini

@@ -0,0 +1,11 @@
+# SPDX-FileCopyrightText: 2023 Vladimír Kotal
+#
+# SPDX-License-Identifier: MIT
+
+[tox]
+envlist = py39
+
+[testenv]
+changedir = {toxinidir}/tests
+deps = pytest==6.2.5
+commands = pytest -v