From cb65cded8825eba47d5fde6f4542e66af7807b04 Mon Sep 17 00:00:00 2001
From: Thales Macedo Garitezi <thalesmg@gmail.com>
Date: Wed, 22 Mar 2023 15:22:13 -0300
Subject: [PATCH] fix(last_will_testament): don't publish LWT if client is
 banned when kicked

Fixes https://emqx.atlassian.net/browse/EMQX-9288

Related issue:
https://github.com/emqx/emqx/issues/10192#issuecomment-1478809900
---
 apps/emqx/src/emqx_channel.erl            | 20 ++++---
 apps/emqx_authz/test/emqx_authz_SUITE.erl | 65 +++++++++++++++++++++++
 changes/ce/fix-10209.en.md                |  2 +
 3 files changed, 80 insertions(+), 7 deletions(-)
 create mode 100644 changes/ce/fix-10209.en.md

diff --git a/apps/emqx/src/emqx_channel.erl b/apps/emqx/src/emqx_channel.erl
index 9acad4d57..e01a16f83 100644
--- a/apps/emqx/src/emqx_channel.erl
+++ b/apps/emqx/src/emqx_channel.erl
@@ -2128,17 +2128,23 @@ publish_will_msg(
     ClientInfo = #{mountpoint := MountPoint},
     Msg = #message{topic = Topic}
 ) ->
-    case emqx_access_control:authorize(ClientInfo, publish, Topic) of
-        allow ->
-            NMsg = emqx_mountpoint:mount(MountPoint, Msg),
-            _ = emqx_broker:publish(NMsg),
-            ok;
-        deny ->
+    PublishingDisallowed = emqx_access_control:authorize(ClientInfo, publish, Topic) =/= allow,
+    ClientBanned = emqx_banned:check(ClientInfo),
+    case PublishingDisallowed orelse ClientBanned of
+        true ->
             ?tp(
                 warning,
                 last_will_testament_publish_denied,
-                #{topic => Topic}
+                #{
+                    topic => Topic,
+                    client_banned => ClientBanned,
+                    publishing_disallowed => PublishingDisallowed
+                }
             ),
+            ok;
+        false ->
+            NMsg = emqx_mountpoint:mount(MountPoint, Msg),
+            _ = emqx_broker:publish(NMsg),
             ok
     end.
 
diff --git a/apps/emqx_authz/test/emqx_authz_SUITE.erl b/apps/emqx_authz/test/emqx_authz_SUITE.erl
index b3ce04f43..84b1d903e 100644
--- a/apps/emqx_authz/test/emqx_authz_SUITE.erl
+++ b/apps/emqx_authz/test/emqx_authz_SUITE.erl
@@ -26,6 +26,8 @@
 -include_lib("emqx/include/emqx_placeholder.hrl").
 -include_lib("snabbkaffe/include/snabbkaffe.hrl").
 
+-import(emqx_common_test_helpers, [on_exit/1]).
+
 all() ->
     emqx_common_test_helpers:all(?MODULE).
 
@@ -65,6 +67,7 @@ end_per_suite(_Config) ->
 
 init_per_testcase(TestCase, Config) when
     TestCase =:= t_subscribe_deny_disconnect_publishes_last_will_testament;
+    TestCase =:= t_publish_last_will_testament_banned_client_connecting;
     TestCase =:= t_publish_deny_disconnect_publishes_last_will_testament
 ->
     {ok, _} = emqx_authz:update(?CMD_REPLACE, []),
@@ -76,11 +79,15 @@ init_per_testcase(_, Config) ->
 
 end_per_testcase(TestCase, _Config) when
     TestCase =:= t_subscribe_deny_disconnect_publishes_last_will_testament;
+    TestCase =:= t_publish_last_will_testament_banned_client_connecting;
     TestCase =:= t_publish_deny_disconnect_publishes_last_will_testament
 ->
     {ok, _} = emqx:update_config([authorization, deny_action], ignore),
+    {ok, _} = emqx_authz:update(?CMD_REPLACE, []),
+    emqx_common_test_helpers:call_janitor(),
     ok;
 end_per_testcase(_TestCase, _Config) ->
+    emqx_common_test_helpers:call_janitor(),
     ok.
 
 set_special_configs(emqx_authz) ->
@@ -396,5 +403,63 @@ t_publish_last_will_testament_denied_topic(_Config) ->
 
     ok.
 
+%% client is allowed by ACL to publish to its LWT topic, is connected,
+%% and then gets banned and kicked out while connected.  Should not
+%% publish LWT.
+t_publish_last_will_testament_banned_client_connecting(_Config) ->
+    {ok, _} = emqx_authz:update(?CMD_REPLACE, [?SOURCE7]),
+    Username = <<"some_client">>,
+    ClientId = <<"some_clientid">>,
+    LWTPayload = <<"should not be published">>,
+    LWTTopic = <<"some_client/lwt">>,
+    ok = emqx:subscribe(<<"some_client/lwt">>),
+    {ok, C} = emqtt:start_link([
+        {clientid, ClientId},
+        {username, Username},
+        {will_topic, LWTTopic},
+        {will_payload, LWTPayload}
+    ]),
+    ?assertMatch({ok, _}, emqtt:connect(C)),
+
+    %% Now we ban the client while it is connected.
+    Now = erlang:system_time(second),
+    Who = {username, Username},
+    emqx_banned:create(#{
+        who => Who,
+        by => <<"test">>,
+        reason => <<"test">>,
+        at => Now,
+        until => Now + 120
+    }),
+    on_exit(fun() -> emqx_banned:delete(Who) end),
+    %% Now kick it as we do in the ban API.
+    process_flag(trap_exit, true),
+    ?check_trace(
+        begin
+            ok = emqx_cm:kick_session(ClientId),
+            receive
+                {deliver, LWTTopic, #message{payload = LWTPayload}} ->
+                    error(lwt_should_not_be_published_to_forbidden_topic)
+            after 2_000 -> ok
+            end,
+            ok
+        end,
+        fun(Trace) ->
+            ?assertMatch(
+                [
+                    #{
+                        client_banned := true,
+                        publishing_disallowed := false
+                    }
+                ],
+                ?of_kind(last_will_testament_publish_denied, Trace)
+            ),
+            ok
+        end
+    ),
+    ok = snabbkaffe:stop(),
+
+    ok.
+
 stop_apps(Apps) ->
     lists:foreach(fun application:stop/1, Apps).
diff --git a/changes/ce/fix-10209.en.md b/changes/ce/fix-10209.en.md
new file mode 100644
index 000000000..21ce98e44
--- /dev/null
+++ b/changes/ce/fix-10209.en.md
@@ -0,0 +1,2 @@
+Fix bug where a last will testament (LWT) message could be published
+when kicking out a banned client.