From 7cd35c9d44b5b951101574eeee5533c7ebfdcb3f Mon Sep 17 00:00:00 2001
From: Thales Macedo Garitezi <thalesmg@gmail.com>
Date: Fri, 10 Jun 2022 16:56:26 -0300
Subject: [PATCH 1/2] fix(metrics): inc `connack.auth_error` when using MQTT
 3.1 (5.0)

Since MQTT 3.1 uses a different reason code for auth failures, it was
failing to increase the corresponding metric that works for MQTT 5.0.
---
 apps/emqx/src/emqx_metrics.erl       |  8 +++++--
 apps/emqx/test/emqx_broker_SUITE.erl | 35 ++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/apps/emqx/src/emqx_metrics.erl b/apps/emqx/src/emqx_metrics.erl
index 8aabd7ac8..7418573dc 100644
--- a/apps/emqx/src/emqx_metrics.erl
+++ b/apps/emqx/src/emqx_metrics.erl
@@ -484,8 +484,12 @@ inc_sent(Packet) ->
 
 do_inc_sent(?CONNACK_PACKET(ReasonCode)) ->
     (ReasonCode == ?RC_SUCCESS) orelse inc('packets.connack.error'),
-    (ReasonCode == ?RC_NOT_AUTHORIZED) andalso inc('packets.connack.auth_error'),
-    (ReasonCode == ?RC_BAD_USER_NAME_OR_PASSWORD) andalso inc('packets.connack.auth_error'),
+    ((ReasonCode == ?RC_NOT_AUTHORIZED) orelse
+        (ReasonCode == ?CONNACK_AUTH)) andalso
+        inc('packets.connack.auth_error'),
+    ((ReasonCode == ?RC_BAD_USER_NAME_OR_PASSWORD) orelse
+        (ReasonCode == ?CONNACK_CREDENTIALS)) andalso
+        inc('packets.connack.auth_error'),
     inc('packets.connack.sent');
 do_inc_sent(?PUBLISH_PACKET(QoS)) ->
     inc('messages.sent'),
diff --git a/apps/emqx/test/emqx_broker_SUITE.erl b/apps/emqx/test/emqx_broker_SUITE.erl
index 398d81614..2de7d9ad5 100644
--- a/apps/emqx/test/emqx_broker_SUITE.erl
+++ b/apps/emqx/test/emqx_broker_SUITE.erl
@@ -678,6 +678,41 @@ t_connect_client_never_negative(Config) when is_list(Config) ->
 t_connect_client_never_negative({'end', _Config}) ->
     ok.
 
+t_connack_auth_error({init, Config}) ->
+    process_flag(trap_exit, true),
+    ok = emqx_common_test_helpers:start_apps([]),
+    ChainName = 'mqtt:global',
+    AuthenticatorConfig = #{
+        enable => true,
+        mechanism => password_based,
+        backend => built_in_database,
+        user_id_type => username,
+        password_hash_algorithm => #{
+            name => plain,
+            salt_position => disable
+        },
+        user_group => <<"global:mqtt">>
+    },
+    ok = emqx_authentication:register_providers(
+        [{{password_based, built_in_database}, emqx_authn_mnesia}]
+    ),
+    emqx_authentication:initialize_authentication(ChainName, AuthenticatorConfig),
+    Config;
+t_connack_auth_error({'end', _Config}) ->
+    ok = emqx_common_test_helpers:stop_apps([]),
+    ok;
+t_connack_auth_error(Config) when is_list(Config) ->
+    %% MQTT 3.1
+    ?assertEqual(0, emqx_metrics:val('packets.connack.auth_error')),
+    {ok, C0} = emqtt:start_link([{proto_ver, v4}]),
+    ?assertEqual({error, {unauthorized_client, undefined}}, emqtt:connect(C0)),
+    ?assertEqual(1, emqx_metrics:val('packets.connack.auth_error')),
+    %% MQTT 5.0
+    {ok, C1} = emqtt:start_link([{proto_ver, v5}]),
+    ?assertEqual({error, {not_authorized, #{}}}, emqtt:connect(C1)),
+    ?assertEqual(2, emqx_metrics:val('packets.connack.auth_error')),
+    ok.
+
 wait_for_events(Action, Kinds) ->
     wait_for_events(Action, Kinds, 500).
 

From 5f9778237a317f38618a6d6cc459f77198e21ef3 Mon Sep 17 00:00:00 2001
From: Thales Macedo Garitezi <thalesmg@gmail.com>
Date: Mon, 13 Jun 2022 10:02:43 -0300
Subject: [PATCH 2/2] test(fix): avoid depending on other apps; fix setup

---
 apps/emqx/test/emqx_broker_SUITE.erl | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/apps/emqx/test/emqx_broker_SUITE.erl b/apps/emqx/test/emqx_broker_SUITE.erl
index 2de7d9ad5..41ff43311 100644
--- a/apps/emqx/test/emqx_broker_SUITE.erl
+++ b/apps/emqx/test/emqx_broker_SUITE.erl
@@ -680,7 +680,6 @@ t_connect_client_never_negative({'end', _Config}) ->
 
 t_connack_auth_error({init, Config}) ->
     process_flag(trap_exit, true),
-    ok = emqx_common_test_helpers:start_apps([]),
     ChainName = 'mqtt:global',
     AuthenticatorConfig = #{
         enable => true,
@@ -694,22 +693,25 @@ t_connack_auth_error({init, Config}) ->
         user_group => <<"global:mqtt">>
     },
     ok = emqx_authentication:register_providers(
-        [{{password_based, built_in_database}, emqx_authn_mnesia}]
+        [{{password_based, built_in_database}, emqx_authentication_SUITE}]
     ),
     emqx_authentication:initialize_authentication(ChainName, AuthenticatorConfig),
     Config;
 t_connack_auth_error({'end', _Config}) ->
-    ok = emqx_common_test_helpers:stop_apps([]),
+    ChainName = 'mqtt:global',
+    AuthenticatorID = <<"password_based:built_in_database">>,
+    ok = emqx_authentication:deregister_provider({password_based, built_in_database}),
+    ok = emqx_authentication:delete_authenticator(ChainName, AuthenticatorID),
     ok;
 t_connack_auth_error(Config) when is_list(Config) ->
     %% MQTT 3.1
     ?assertEqual(0, emqx_metrics:val('packets.connack.auth_error')),
     {ok, C0} = emqtt:start_link([{proto_ver, v4}]),
-    ?assertEqual({error, {unauthorized_client, undefined}}, emqtt:connect(C0)),
+    ?assertEqual({error, {malformed_username_or_password, undefined}}, emqtt:connect(C0)),
     ?assertEqual(1, emqx_metrics:val('packets.connack.auth_error')),
     %% MQTT 5.0
     {ok, C1} = emqtt:start_link([{proto_ver, v5}]),
-    ?assertEqual({error, {not_authorized, #{}}}, emqtt:connect(C1)),
+    ?assertEqual({error, {bad_username_or_password, #{}}}, emqtt:connect(C1)),
     ?assertEqual(2, emqx_metrics:val('packets.connack.auth_error')),
     ok.