From d65d690c175fe3e8cdc11a2d6eaa53c392654bb4 Mon Sep 17 00:00:00 2001
From: Stefan Strigler <stefan.strigler@emqx.io>
Date: Thu, 6 Jul 2023 15:26:15 +0200
Subject: [PATCH] fix(emqx_gateway): return 404 for unknown client id

---
 .../src/emqx_gateway_api_clients.erl          | 48 +++++++++++++++++--
 .../test/emqx_gateway_api_SUITE.erl           | 41 ++++++++++++++++
 .../test/emqx_gateway_cli_SUITE.erl           | 16 +------
 .../test/emqx_gateway_test_utils.erl          | 16 +++++++
 4 files changed, 102 insertions(+), 19 deletions(-)

diff --git a/apps/emqx_gateway/src/emqx_gateway_api_clients.erl b/apps/emqx_gateway/src/emqx_gateway_api_clients.erl
index 8037f4197..8cfcb70e6 100644
--- a/apps/emqx_gateway/src/emqx_gateway_api_clients.erl
+++ b/apps/emqx_gateway/src/emqx_gateway_api_clients.erl
@@ -57,7 +57,8 @@
     qs2ms/2,
     run_fuzzy_filter/2,
     format_channel_info/1,
-    format_channel_info/2
+    format_channel_info/2,
+    client_info_mountpoint/1
 ]).
 
 -define(TAGS, [<<"Gateway Clients">>]).
@@ -177,8 +178,12 @@ clients_insta(delete, #{
     }
 }) ->
     with_gateway(Name0, fun(GwName, _) ->
-        _ = emqx_gateway_http:kickout_client(GwName, ClientId),
-        {204}
+        case emqx_gateway_http:kickout_client(GwName, ClientId) of
+            {error, not_found} ->
+                return_http_error(404, "Client not found");
+            _ ->
+                {204}
+        end
     end).
 
 %% List the established subscriptions with mountpoint
@@ -234,8 +239,13 @@ subscriptions(delete, #{
     }
 }) ->
     with_gateway(Name0, fun(GwName, _) ->
-        _ = emqx_gateway_http:client_unsubscribe(GwName, ClientId, Topic),
-        {204}
+        case lookup_topic(GwName, ClientId, Topic) of
+            {ok, _} ->
+                _ = emqx_gateway_http:client_unsubscribe(GwName, ClientId, Topic),
+                {204};
+            {error, not_found} ->
+                return_http_error(404, "Resource not found")
+        end
     end).
 
 %%--------------------------------------------------------------------
@@ -260,6 +270,34 @@ extra_sub_props(Props) ->
         #{subid => maps:get(<<"subid">>, Props, undefined)}
     ).
 
+lookup_topic(GwName, ClientId, Topic) ->
+    Mountpoints = emqx_gateway_http:lookup_client(
+        GwName,
+        ClientId,
+        {?MODULE, client_info_mountpoint}
+    ),
+    case emqx_gateway_http:list_client_subscriptions(GwName, ClientId) of
+        {ok, Subscriptions} ->
+            case
+                [
+                    S
+                 || S = #{topic := Topic0} <- Subscriptions,
+                    Mountpoint <- Mountpoints,
+                    Topic0 == emqx_mountpoint:mount(Mountpoint, Topic)
+                ]
+            of
+                [] ->
+                    {error, not_found};
+                Filtered ->
+                    {ok, Filtered}
+            end;
+        Error ->
+            Error
+    end.
+
+client_info_mountpoint({_, #{clientinfo := #{mountpoint := Mountpoint}}, _}) ->
+    Mountpoint.
+
 %%--------------------------------------------------------------------
 %% QueryString to MatchSpec
 
diff --git a/apps/emqx_gateway/test/emqx_gateway_api_SUITE.erl b/apps/emqx_gateway/test/emqx_gateway_api_SUITE.erl
index e486c8c16..a3fe39852 100644
--- a/apps/emqx_gateway/test/emqx_gateway_api_SUITE.erl
+++ b/apps/emqx_gateway/test/emqx_gateway_api_SUITE.erl
@@ -586,6 +586,47 @@ t_listeners_authn_data_mgmt(_) ->
 
     ok.
 
+t_clients(_) ->
+    GwConf = #{
+        name => <<"mqttsn">>,
+        gateway_id => 1,
+        broadcast => true,
+        predefined => [#{id => 1, topic => <<"t/a">>}],
+        enable_qos3 => true,
+        listeners => [
+            #{name => <<"def">>, type => <<"udp">>, bind => <<"1884">>}
+        ]
+    },
+    init_gw("mqttsn", GwConf),
+    Path = "/gateways/mqttsn/clients",
+    MyClient = Path ++ "/my_client",
+    MyClientSubscriptions = MyClient ++ "/subscriptions",
+    {200, NoClients} = request(get, Path),
+    ?assertMatch(#{data := []}, NoClients),
+
+    ClientSocket = emqx_gateway_test_utils:sn_client_connect(<<"my_client">>),
+    {200, _} = request(get, MyClient),
+    {200, Clients} = request(get, Path),
+    ?assertMatch(#{data := [#{clientid := <<"my_client">>}]}, Clients),
+
+    {201, _} = request(post, MyClientSubscriptions, #{topic => <<"test/topic">>}),
+    {200, Subscriptions} = request(get, MyClientSubscriptions),
+    ?assertMatch([#{topic := <<"test/topic">>}], Subscriptions),
+    {204, _} = request(delete, MyClientSubscriptions ++ "/test%2Ftopic"),
+    {200, []} = request(get, MyClientSubscriptions),
+    {404, _} = request(delete, MyClientSubscriptions ++ "/test%2Ftopic"),
+
+    {204, _} = request(delete, MyClient),
+    {404, _} = request(delete, MyClient),
+    {404, _} = request(get, MyClient),
+    {404, _} = request(get, MyClientSubscriptions),
+    {404, _} = request(post, MyClientSubscriptions, #{topic => <<"foo">>}),
+    {404, _} = request(delete, MyClientSubscriptions ++ "/topic"),
+    {200, NoClients2} = request(get, Path),
+    ?assertMatch(#{data := []}, NoClients2),
+    emqx_gateway_test_utils:sn_client_disconnect(ClientSocket),
+    ok.
+
 t_authn_fuzzy_search(_) ->
     init_gw("stomp"),
     AuthConf = #{
diff --git a/apps/emqx_gateway/test/emqx_gateway_cli_SUITE.erl b/apps/emqx_gateway/test/emqx_gateway_cli_SUITE.erl
index 641528eda..fdaa55ab8 100644
--- a/apps/emqx_gateway/test/emqx_gateway_cli_SUITE.erl
+++ b/apps/emqx_gateway/test/emqx_gateway_cli_SUITE.erl
@@ -54,6 +54,8 @@ end).
     "}\n"
 ).
 
+-import(emqx_gateway_test_utils, [sn_client_connect/1, sn_client_disconnect/1]).
+
 %%--------------------------------------------------------------------
 %% Setup
 %%--------------------------------------------------------------------
@@ -303,17 +305,3 @@ acc_print(Acc) ->
     after 200 ->
         Acc
     end.
-
-sn_client_connect(ClientId) ->
-    {ok, Socket} = gen_udp:open(0, [binary]),
-    _ = emqx_sn_protocol_SUITE:send_connect_msg(Socket, ClientId),
-    ?assertEqual(
-        <<3, 16#05, 0>>,
-        emqx_sn_protocol_SUITE:receive_response(Socket)
-    ),
-    Socket.
-
-sn_client_disconnect(Socket) ->
-    _ = emqx_sn_protocol_SUITE:send_disconnect_msg(Socket, undefined),
-    gen_udp:close(Socket),
-    ok.
diff --git a/apps/emqx_gateway/test/emqx_gateway_test_utils.erl b/apps/emqx_gateway/test/emqx_gateway_test_utils.erl
index 2e8a3a583..950ae1bcf 100644
--- a/apps/emqx_gateway/test/emqx_gateway_test_utils.erl
+++ b/apps/emqx_gateway/test/emqx_gateway_test_utils.erl
@@ -19,6 +19,8 @@
 -compile(export_all).
 -compile(nowarn_export_all).
 
+-include_lib("eunit/include/eunit.hrl").
+
 assert_confs(Expected0, Effected) ->
     Expected = maybe_unconvert_listeners(Expected0),
     case do_assert_confs(root, Expected, Effected) of
@@ -181,3 +183,17 @@ url(Path, Qs) ->
 
 auth(Headers) ->
     [emqx_mgmt_api_test_util:auth_header_() | Headers].
+
+sn_client_connect(ClientId) ->
+    {ok, Socket} = gen_udp:open(0, [binary]),
+    _ = emqx_sn_protocol_SUITE:send_connect_msg(Socket, ClientId),
+    ?assertEqual(
+        <<3, 16#05, 0>>,
+        emqx_sn_protocol_SUITE:receive_response(Socket)
+    ),
+    Socket.
+
+sn_client_disconnect(Socket) ->
+    _ = emqx_sn_protocol_SUITE:send_disconnect_msg(Socket, undefined),
+    gen_udp:close(Socket),
+    ok.