From 0d2ce85776c5cba495489090cd7ef5dd31c8c74c Mon Sep 17 00:00:00 2001
From: Stefan Strigler <stefan.strigler@emqx.io>
Date: Thu, 16 Feb 2023 17:39:49 +0100
Subject: [PATCH] fix: return 'not found' for subscriptions of unknown client

---
 apps/emqx_management/src/emqx_mgmt.erl        | 33 +++++++++++--------
 .../src/emqx_mgmt_api_clients.erl             |  9 ++---
 apps/emqx_management/test/emqx_mgmt_SUITE.erl | 21 ++++++++++++
 .../test/emqx_mgmt_api_clients_SUITE.erl      |  2 +-
 4 files changed, 46 insertions(+), 19 deletions(-)

diff --git a/apps/emqx_management/src/emqx_mgmt.erl b/apps/emqx_management/src/emqx_mgmt.erl
index 04f7d9ad0..31436b9a0 100644
--- a/apps/emqx_management/src/emqx_mgmt.erl
+++ b/apps/emqx_management/src/emqx_mgmt.erl
@@ -286,12 +286,12 @@ maybe_format(undefined, A) ->
 maybe_format({M, F}, A) ->
     M:F(A).
 
-kickout_client(ClientID) ->
-    case lookup_client({clientid, ClientID}, undefined) of
+kickout_client(ClientId) ->
+    case lookup_client({clientid, ClientId}, undefined) of
         [] ->
             {error, not_found};
         _ ->
-            Results = [kickout_client(Node, ClientID) || Node <- mria_mnesia:running_nodes()],
+            Results = [kickout_client(Node, ClientId) || Node <- mria_mnesia:running_nodes()],
             check_results(Results)
     end.
 
@@ -302,17 +302,22 @@ list_authz_cache(ClientId) ->
     call_client(ClientId, list_authz_cache).
 
 list_client_subscriptions(ClientId) ->
-    Results = [client_subscriptions(Node, ClientId) || Node <- mria_mnesia:running_nodes()],
-    Filter =
-        fun
-            ({error, _}) ->
-                false;
-            ({_Node, List}) ->
-                erlang:is_list(List) andalso 0 < erlang:length(List)
-        end,
-    case lists:filter(Filter, Results) of
-        [] -> [];
-        [Result | _] -> Result
+    case lookup_client({clientid, ClientId}, undefined) of
+        [] ->
+            {error, not_found};
+        _ ->
+            Results = [client_subscriptions(Node, ClientId) || Node <- mria_mnesia:running_nodes()],
+            Filter =
+                fun
+                    ({error, _}) ->
+                        false;
+                    ({_Node, List}) ->
+                        erlang:is_list(List) andalso 0 < erlang:length(List)
+                end,
+            case lists:filter(Filter, Results) of
+                [] -> [];
+                [Result | _] -> Result
+            end
     end.
 
 client_subscriptions(Node, ClientId) ->
diff --git a/apps/emqx_management/src/emqx_mgmt_api_clients.erl b/apps/emqx_management/src/emqx_mgmt_api_clients.erl
index 0a5488389..cac3edaed 100644
--- a/apps/emqx_management/src/emqx_mgmt_api_clients.erl
+++ b/apps/emqx_management/src/emqx_mgmt_api_clients.erl
@@ -274,11 +274,10 @@ schema("/clients/:clientid/subscriptions") ->
             responses => #{
                 200 => hoconsc:mk(
                     hoconsc:array(hoconsc:ref(emqx_mgmt_api_subscriptions, subscription)), #{}
+                ),
+                404 => emqx_dashboard_swagger:error_codes(
+                    ['CLIENTID_NOT_FOUND'], <<"Client ID not found">>
                 )
-                %% returns [] if client not existed in cluster
-                %404 => emqx_dashboard_swagger:error_codes(
-                %    ['CLIENTID_NOT_FOUND'], <<"Client ID not found">>
-                %)
             }
         }
     };
@@ -599,6 +598,8 @@ unsubscribe_batch(post, #{bindings := #{clientid := ClientID}, body := TopicInfo
 
 subscriptions(get, #{bindings := #{clientid := ClientID}}) ->
     case emqx_mgmt:list_client_subscriptions(ClientID) of
+        {error, not_found} ->
+            {404, ?CLIENTID_NOT_FOUND};
         [] ->
             {200, []};
         {Node, Subs} ->
diff --git a/apps/emqx_management/test/emqx_mgmt_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_SUITE.erl
index 13acb9fc3..a9379da8c 100644
--- a/apps/emqx_management/test/emqx_mgmt_SUITE.erl
+++ b/apps/emqx_management/test/emqx_mgmt_SUITE.erl
@@ -145,6 +145,27 @@ t_kickout_client(Config) ->
     end,
     ?assertEqual({error, not_found}, emqx_mgmt:kickout_client(<<"notfound">>)).
 
+t_list_authz_cache(init, Config) ->
+    setup_clients(Config);
+t_list_authz_cache('end', Config) ->
+    disconnect_clients(Config).
+
+t_list_authz_cache(_) ->
+    ?assertNotMatch({error, _}, emqx_mgmt:list_authz_cache(<<"client1">>)),
+    ?assertMatch({error, not_found}, emqx_mgmt:list_authz_cache(<<"notfound">>)).
+
+t_list_client_subscriptions(init, Config) ->
+    setup_clients(Config);
+t_list_client_subscriptions('end', Config) ->
+    disconnect_clients(Config).
+
+t_list_client_subscriptions(Config) ->
+    [Client | _] = ?config(clients, Config),
+    ?assertEqual([], emqx_mgmt:list_client_subscriptions(<<"client1">>)),
+    emqtt:subscribe(Client, <<"t/#">>),
+    ?assertMatch({_, [{<<"t/#">>, _Opts}]}, emqx_mgmt:list_client_subscriptions(<<"client1">>)),
+    ?assertEqual({error, not_found}, emqx_mgmt:list_client_subscriptions(<<"notfound">>)).
+
 %%% helpers
 ident(Arg) ->
     Arg.
diff --git a/apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl
index d843a2209..b2f3f5655 100644
--- a/apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl
+++ b/apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl
@@ -279,7 +279,7 @@ t_client_id_not_found(_Config) ->
     %% Client kickout
     ?assertMatch({error, {Http, _, Body}}, ReqFun(delete, PathFun([]))),
     %% Client Subscription list
-    ?assertMatch({ok, {{"HTTP/1.1", 200, "OK"}, _, "[]"}}, ReqFun(get, PathFun(["subscriptions"]))),
+    ?assertMatch({error, {Http, _, Body}}, ReqFun(get, PathFun(["subscriptions"]))),
     %% AuthZ Cache lookup
     ?assertMatch({error, {Http, _, Body}}, ReqFun(get, PathFun(["authorization", "cache"]))),
     %% AuthZ Cache clean