From 3fd28f9e182ff34d96d85e5f01c6885ab06e562e Mon Sep 17 00:00:00 2001
From: Stefan Strigler <stefan.strigler@emqx.io>
Date: Fri, 7 Jul 2023 11:57:33 +0200
Subject: [PATCH] fix(emqx_management): return 404 for unknown listener id

---
 .../src/emqx_mgmt_api_listeners.erl           | 44 +++++++++----------
 .../test/emqx_mgmt_api_listeners_SUITE.erl    |  7 ++-
 2 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/apps/emqx_management/src/emqx_mgmt_api_listeners.erl b/apps/emqx_management/src/emqx_mgmt_api_listeners.erl
index dd9013b16..8c20cbab8 100644
--- a/apps/emqx_management/src/emqx_mgmt_api_listeners.erl
+++ b/apps/emqx_management/src/emqx_mgmt_api_listeners.erl
@@ -45,14 +45,10 @@
     update/3
 ]).
 
--include_lib("emqx/include/emqx.hrl").
 -include_lib("hocon/include/hoconsc.hrl").
 
--define(NODE_LISTENER_NOT_FOUND, <<"Node name or listener id not found">>).
--define(NODE_NOT_FOUND_OR_DOWN, <<"Node not found or Down">>).
 -define(LISTENER_NOT_FOUND, <<"Listener id not found">>).
 -define(LISTENER_ID_INCONSISTENT, <<"Path and body's listener id not match">>).
--define(ADDR_PORT_INUSE, <<"Addr port in use">>).
 
 namespace() -> "listeners".
 
@@ -156,7 +152,7 @@ schema("/listeners/:id") ->
             parameters => [?R_REF(listener_id)],
             responses => #{
                 204 => <<"Listener deleted">>,
-                400 => error_codes(['BAD_REQUEST'])
+                404 => error_codes(['BAD_LISTENER_ID'])
             }
         }
     };
@@ -405,20 +401,8 @@ list_listeners(get, #{query_string := Query}) ->
 list_listeners(post, #{body := Body}) ->
     create_listener(Body).
 
-crud_listeners_by_id(get, #{bindings := #{id := Id0}}) ->
-    Listeners =
-        [
-            Conf#{
-                <<"id">> => Id,
-                <<"type">> => Type,
-                <<"bind">> := iolist_to_binary(
-                    emqx_listeners:format_bind(maps:get(<<"bind">>, Conf))
-                )
-            }
-         || {Id, Type, Conf} <- emqx_listeners:list_raw(),
-            Id =:= Id0
-        ],
-    case Listeners of
+crud_listeners_by_id(get, #{bindings := #{id := Id}}) ->
+    case find_listeners_by_id(Id) of
         [] -> {404, #{code => 'BAD_LISTENER_ID', message => ?LISTENER_NOT_FOUND}};
         [L] -> {200, L}
     end;
@@ -449,9 +433,12 @@ crud_listeners_by_id(post, #{body := Body}) ->
     create_listener(Body);
 crud_listeners_by_id(delete, #{bindings := #{id := Id}}) ->
     {ok, #{type := Type, name := Name}} = emqx_listeners:parse_listener_id(Id),
-    case ensure_remove(Type, Name) of
-        {ok, _} -> {204};
-        {error, Reason} -> {400, #{code => 'BAD_REQUEST', message => err_msg(Reason)}}
+    case find_listeners_by_id(Id) of
+        [_L] ->
+            {ok, _Res} = ensure_remove(Type, Name),
+            {204};
+        [] ->
+            {404, #{code => 'BAD_LISTENER_ID', message => ?LISTENER_NOT_FOUND}}
     end.
 
 parse_listener_conf(Conf0) ->
@@ -585,6 +572,19 @@ do_list_listeners() ->
         <<"listeners">> => Listeners
     }.
 
+find_listeners_by_id(Id) ->
+    [
+        Conf#{
+            <<"id">> => Id0,
+            <<"type">> => Type,
+            <<"bind">> := iolist_to_binary(
+                emqx_listeners:format_bind(maps:get(<<"bind">>, Conf))
+            )
+        }
+     || {Id0, Type, Conf} <- emqx_listeners:list_raw(),
+        Id0 =:= Id
+    ].
+
 wrap_rpc({badrpc, Reason}) ->
     {error, Reason};
 wrap_rpc(Res) ->
diff --git a/apps/emqx_management/test/emqx_mgmt_api_listeners_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_api_listeners_SUITE.erl
index 862d81ab8..96ec8f2de 100644
--- a/apps/emqx_management/test/emqx_mgmt_api_listeners_SUITE.erl
+++ b/apps/emqx_management/test/emqx_mgmt_api_listeners_SUITE.erl
@@ -399,12 +399,15 @@ crud_listeners_by_id(ListenerId, NewListenerId, MinListenerId, BadId, Type, Port
     ?assertEqual([], delete(MinPath)),
     ?assertEqual({error, not_found}, is_running(NewListenerId)),
     ?assertMatch({error, {"HTTP/1.1", 404, _}}, request(get, NewPath, [], [])),
-    ?assertEqual([], delete(NewPath)),
+    ?assertMatch({error, {"HTTP/1.1", 404, _}}, request(delete, NewPath, [], [])),
     ok.
 
 t_delete_nonexistent_listener(Config) when is_list(Config) ->
     NonExist = emqx_mgmt_api_test_util:api_path(["listeners", "tcp:nonexistent"]),
-    ?assertEqual([], delete(NonExist)),
+    ?assertMatch(
+        {error, {_, 404, _}},
+        request(delete, NonExist, [], [])
+    ),
     ok.
 
 t_action_listeners(Config) when is_list(Config) ->