Add more tests (#61)

Namoshek · web-flow · commit b155078f8e4c · 2021-01-10T11:33:40.000+01:00
* Add tests for remaining TLS options

* Replace test pipeline badge with coverage

* Test connecting to TLS endpoint without config

* Simplify exit-when-queues-empty condition

* Raise notice if socket could not be closed

* Test unregistering single message received event handler

* Try EMQ X with longer erlang startup time

* Switch back to updated v1 of action
diff --git a/.ci/tls/.gitignore b/.ci/tls/.gitignore
@@ -1,6 +1,2 @@
-*.crt
-*.csr
-*.jks
-*.key
-*.p12
-*.srl
+*
+!.gitignore
diff --git a/README.md b/README.md
@@ -2,7 +2,7 @@
 
 [![Latest Stable Version](https://poser.pugx.org/php-mqtt/client/v)](https://packagist.org/packages/php-mqtt/client)
 [![Total Downloads](https://poser.pugx.org/php-mqtt/client/downloads)](https://packagist.org/packages/php-mqtt/client)
-[![Tests](https://github.com/php-mqtt/client/workflows/Tests/badge.svg)](https://github.com/php-mqtt/client/actions?query=workflow%3ATests)
+[![Coverage](https://sonarcloud.io/api/project_badges/measure?project=php-mqtt_client&metric=coverage)](https://sonarcloud.io/dashboard?id=php-mqtt_client)
 [![Quality Gate Status](https://sonarcloud.io/api/project_badges/measure?project=php-mqtt_client&metric=alert_status)](https://sonarcloud.io/dashboard?id=php-mqtt_client)
 [![Maintainability Rating](https://sonarcloud.io/api/project_badges/measure?project=php-mqtt_client&metric=sqale_rating)](https://sonarcloud.io/dashboard?id=php-mqtt_client)
 [![Reliability Rating](https://sonarcloud.io/api/project_badges/measure?project=php-mqtt_client&metric=reliability_rating)](https://sonarcloud.io/dashboard?id=php-mqtt_client)
@@ -195,6 +195,27 @@ $connectionSettings = (new \PhpMqtt\Client\ConnectionSettings)
 - Message flows with a QoS level higher than 0 are not persisted as the default implementation uses an in-memory repository for data.
   To avoid issues with broken message flows, use the clean session flag to indicate that you don't care about old data.
   It will not only instruct the broker to consider the connection new (without previous state), but will also reset the registered repository.
+  
+## Developing & Testing
+
+### Certificates (TLS)
+
+To run the tests (especially the TLS tests), you will need to create certificates. A command has been provided for this:
+```sh
+sh create-certificates.sh
+```
+This will create all required certificates in the `.ci/tls/` directory. The same script is used for continuous integration as well.
+
+### MQTT Broker for Testing
+
+Running the tests expects an MQTT broker to be running. The easiest way to run an MQTT broker is through Docker:
+```sh
+docker run --rm -it -p 1883:1883 -p 8883:8883 -p 8884:8884 -v $(pwd)/.ci/tls:/mosquitto-certs -v $(pwd)/.ci/mosquitto.conf:/mosquitto/config/mosquitto.conf eclipse-mosquitto:1.6
+```
+When run from the project directory, this will spawn a Mosquitto MQTT broker configured with the generated TLS certificates and a custom configuration.
+
+In case you intend to run a different broker or using a different method, or use a public broker instead,
+you will need to adjust the environment variables defined in `phpunit.xml` accordingly.
 
 ## License
 
diff --git a/create-certificates.sh b/create-certificates.sh
@@ -1,15 +1,30 @@
 #!/bin/sh
 
+# Generate a new CA certificate and key.
 openssl genrsa -out .ci/tls/ca.key 2048
 openssl req -x509 -new -nodes -key .ci/tls/ca.key -days 1 -out .ci/tls/ca.crt -subj "/C=AT/ST=Vorarlberg/CN=php-mqtt Test CA"
+
+# Copy ca.crt to a file named by the hashed subject of the certificate. This is required for PHP's capath option to find the certificate.
+cp .ci/tls/ca.crt .ci/tls/$(openssl x509 -hash -noout -in .ci/tls/ca.crt).0
+
+# Create a Java Trust Store from the CA certificate. This is used by HiveMQ.
 keytool -import -file .ci/tls/ca.crt -alias ca -keystore .ci/tls/ca.jks -storepass s3cr3t -trustcacerts -noprompt
 
+# Generate a new server certificate and key, signed by the created CA.
 openssl genrsa -out .ci/tls/server.key 2048
 openssl req -new -key .ci/tls/server.key -out .ci/tls/server.csr -sha512 -subj "/C=AT/ST=Vorarlberg/CN=localhost"
 openssl x509 -req -in .ci/tls/server.csr -CA .ci/tls/ca.crt -CAkey .ci/tls/ca.key -CAcreateserial -out .ci/tls/server.crt -days 1 -sha512
+
+# Generate a Java Key Store from the server certificate. This is used by HiveMQ.
 openssl pkcs12 -export -in .ci/tls/server.crt -inkey .ci/tls/server.key -out .ci/tls/server.p12 -passout pass:s3cr3t
 keytool -importkeystore -srckeystore .ci/tls/server.p12 -srcstoretype PKCS12 -destkeystore .ci/tls/server.jks -deststoretype JKS -srcstorepass s3cr3t -deststorepass s3cr3t -noprompt
 
+# Generate a client certificate without passphrase, signed by the created CA.
 openssl genrsa -out .ci/tls/client.key 2048
 openssl req -new -key .ci/tls/client.key -out .ci/tls/client.csr -sha512 -subj "/C=AT/ST=Vorarlberg/CN=localhost"
 openssl x509 -req -in .ci/tls/client.csr -CA .ci/tls/ca.crt -CAkey .ci/tls/ca.key -CAcreateserial -out .ci/tls/client.crt -days 1 -sha256
+
+# Generate a client certificate with passphrase, signed by the created CA.
+openssl genrsa -aes128 -passout pass:s3cr3t -out .ci/tls/client2.key 2048
+openssl req -new -key .ci/tls/client2.key -passin pass:s3cr3t -out .ci/tls/client2.csr -sha512 -subj "/C=AT/ST=Vorarlberg/CN=localhost"
+openssl x509 -req -in .ci/tls/client2.csr -CA .ci/tls/ca.crt -CAkey .ci/tls/ca.key -CAcreateserial -out .ci/tls/client2.crt -days 1 -sha256
diff --git a/src/MqttClient.php b/src/MqttClient.php
@@ -603,18 +603,16 @@ public function loop(bool $allowSleep = true, bool $exitWhenQueuesEmpty = false,
                 $this->ping();
             }
 
-            // This check will ensure, that, if we want to exit as soon as all queues
-            // are empty and they really are empty, we quit.
-            if ($exitWhenQueuesEmpty) {
-                if ($this->allQueuesAreEmpty() && $this->repository->countSubscriptions() === 0) {
+            // If configured, the loop is exited if all queues are empty or a certain time limit is reached (i.e. retry is aborted).
+            // In any case, there may not be any active subscriptions though.
+            if ($exitWhenQueuesEmpty && $this->repository->countSubscriptions() === 0) {
+                if ($this->allQueuesAreEmpty()) {
                     break;
                 }
 
-                // We also exit the loop if there are no open topic subscriptions
-                // and we reached the time limit.
-                if ($queueWaitLimit !== null &&
-                    (microtime(true) - $loopStartedAt) > $queueWaitLimit &&
-                    $this->repository->countSubscriptions() === 0) {
+                // The time limit is reached. This most likely means the outgoing queues could not be emptied in time.
+                // Probably the server did not respond with an acknowledgement.
+                if ($queueWaitLimit !== null && (microtime(true) - $loopStartedAt) > $queueWaitLimit) {
                     break;
                 }
             }
@@ -1139,7 +1137,7 @@ protected function closeSocket(): void
             $this->logger->debug('Successfully closed socket connection to the broker.');
         } else {
             $phpError = error_get_last();
-            $this->logger->debug('Closing socket connection failed: {error}', [
+            $this->logger->notice('Closing socket connection failed: {error}', [
                 'error' => $phpError ? $phpError['message'] : 'undefined',
             ]);
         }
diff --git a/tests/Feature/ConnectWithTlsSettingsTest.php b/tests/Feature/ConnectWithTlsSettingsTest.php
@@ -7,6 +7,7 @@
 namespace Tests\Feature;
 
 use PhpMqtt\Client\ConnectionSettings;
+use PhpMqtt\Client\Exceptions\ConnectingToBrokerFailedException;
 use PhpMqtt\Client\MqttClient;
 use Tests\TestCase;
 
@@ -26,6 +27,19 @@ protected function setUp(): void
         }
     }
 
+    public function test_connecting_with_tls_but_without_further_configuration_throws_for_self_signed_certificate(): void
+    {
+        $client = new MqttClient($this->mqttBrokerHost, $this->mqttBrokerTlsPort, 'test-tls-settings');
+
+        $connectionSettings = (new ConnectionSettings)
+            ->setUseTls(true);
+
+        $this->expectException(ConnectingToBrokerFailedException::class);
+        $this->expectExceptionCode(ConnectingToBrokerFailedException::EXCEPTION_CONNECTION_TLS_ERROR);
+
+        $client->connect($connectionSettings, true);
+    }
+
     public function test_connecting_with_tls_with_ignored_self_signed_certificate_works_as_intended(): void
     {
         $client = new MqttClient($this->mqttBrokerHost, $this->mqttBrokerTlsPort, 'test-tls-settings');
@@ -43,7 +57,7 @@ public function test_connecting_with_tls_with_ignored_self_signed_certificate_wo
         $client->disconnect();
     }
 
-    public function test_connecting_with_tls_with_validated_self_signed_certificate_works_as_intended(): void
+    public function test_connecting_with_tls_with_validated_self_signed_certificate_using_cafile__works_as_intended(): void
     {
         $client = new MqttClient($this->mqttBrokerHost, $this->mqttBrokerTlsPort, 'test-tls-settings');
 
@@ -61,6 +75,24 @@ public function test_connecting_with_tls_with_validated_self_signed_certificate_
         $client->disconnect();
     }
 
+    public function test_connecting_with_tls_with_validated_self_signed_certificate_using_capath_works_as_intended(): void
+    {
+        $client = new MqttClient($this->mqttBrokerHost, $this->mqttBrokerTlsPort, 'test-tls-settings');
+
+        $connectionSettings = (new ConnectionSettings)
+            ->setUseTls(true)
+            ->setTlsSelfSignedAllowed(false)
+            ->setTlsVerifyPeer(true)
+            ->setTlsVerifyPeerName(true)
+            ->setTlsCertificateAuthorityPath($this->tlsCertificateDirectory);
+
+        $client->connect($connectionSettings, true);
+
+        $this->assertTrue($client->isConnected());
+
+        $client->disconnect();
+    }
+
     public function test_connecting_with_tls_and_client_certificate_with_validated_self_signed_certificate_works_as_intended(): void
     {
         $client = new MqttClient($this->mqttBrokerHost, $this->mqttBrokerTlsWithClientCertificatePort, 'test-tls-settings');
@@ -80,4 +112,25 @@ public function test_connecting_with_tls_and_client_certificate_with_validated_s
 
         $client->disconnect();
     }
+
+    public function test_connecting_with_tls_and_passphrase_protected_client_certificate_with_validated_self_signed_certificate_works_as_intended(): void
+    {
+        $client = new MqttClient($this->mqttBrokerHost, $this->mqttBrokerTlsWithClientCertificatePort, 'test-tls-settings');
+
+        $connectionSettings = (new ConnectionSettings)
+            ->setUseTls(true)
+            ->setTlsSelfSignedAllowed(false)
+            ->setTlsVerifyPeer(true)
+            ->setTlsVerifyPeerName(true)
+            ->setTlsCertificateAuthorityFile($this->tlsCertificateDirectory . '/ca.crt')
+            ->setTlsClientCertificateFile($this->tlsCertificateDirectory . '/client2.crt')
+            ->setTlsClientCertificateKeyFile($this->tlsCertificateDirectory . '/client2.key')
+            ->setTlsClientCertificateKeyPassphrase('s3cr3t');
+
+        $client->connect($connectionSettings, true);
+
+        $this->assertTrue($client->isConnected());
+
+        $client->disconnect();
+    }
 }
diff --git a/tests/Feature/MessageReceivedEventHandlerTest.php b/tests/Feature/MessageReceivedEventHandlerTest.php
@@ -53,15 +53,54 @@ public function test_message_received_event_handlers_are_called_for_each_receive
         $subscriber->disconnect();
     }
 
+    public function test_message_received_event_handler_can_be_unregistered_and_will_not_be_called_anymore(): void
+    {
+        // We connect and subscribe to a topic using the first client.
+        $subscriber = new MqttClient($this->mqttBrokerHost, $this->mqttBrokerPort, 'subscriber');
+        $subscriber->connect(null, true);
+
+        $callCount = 0;
+        $handler = function (MqttClient $client, string $topic, string $message, int $qualityOfService, bool $retained) use (&$handler, &$callCount) {
+            $callCount++;
+
+            $this->assertSame('foo/bar/baz/01', $topic);
+            $this->assertSame('hello world', $message);
+            $this->assertSame(0, $qualityOfService);
+            $this->assertFalse($retained);
+
+            $client->unregisterMessageReceivedEventHandler($handler);
+            $client->interrupt();
+        };
+
+        $subscriber->registerMessageReceivedEventHandler($handler);
+        $subscriber->subscribe('foo/bar/baz/+');
+
+        // We publish a message from a second client on the same topic.
+        $publisher = new MqttClient($this->mqttBrokerHost, $this->mqttBrokerPort, 'publisher');
+        $publisher->connect(null, true);
+
+        $publisher->publish('foo/bar/baz/01', 'hello world', 0, false);
+        $publisher->publish('foo/bar/baz/02', 'hello world', 0, false);
+
+        // Then we loop on the subscriber to (hopefully) receive the published message.
+        $subscriber->loop(true);
+
+        $this->assertSame(1, $callCount);
+
+        // Finally, we disconnect for a graceful shutdown on the broker side.
+        $publisher->disconnect();
+        $subscriber->disconnect();
+    }
+
     public function test_message_received_event_handlers_can_be_unregistered_and_will_not_be_called_anymore(): void
     {
         // We connect and subscribe to a topic using the first client.
         $subscriber = new MqttClient($this->mqttBrokerHost, $this->mqttBrokerPort, 'subscriber');
         $subscriber->connect(null, true);
 
-        $handlerCallCount = 0;
-        $handler = function (MqttClient $client, string $topic, string $message, int $qualityOfService, bool $retained) use (&$handlerCallCount) {
-            $handlerCallCount++;
+        $callCount = 0;
+        $handler = function (MqttClient $client, string $topic, string $message, int $qualityOfService, bool $retained) use (&$callCount) {
+            $callCount++;
 
             $this->assertSame('foo/bar/baz/01', $topic);
             $this->assertSame('hello world', $message);
@@ -85,7 +124,7 @@ public function test_message_received_event_handlers_can_be_unregistered_and_wil
         // Then we loop on the subscriber to (hopefully) receive the published message.
         $subscriber->loop(true);
 
-        $this->assertSame(1, $handlerCallCount);
+        $this->assertSame(1, $callCount);
 
         // Finally, we disconnect for a graceful shutdown on the broker side.
         $publisher->disconnect();

-Original file line number
+Diff line change
@@ @@ -1,6 +1,2 @@ @@
 -*.crt
 -*.csr
 -*.jks
 -*.key
 -*.p12
 -*.srl
 +*
 +!.gitignore