From 3bc419ee64a33b9ee01ddff47075dc41ed64909a Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Thu, 6 Jul 2023 15:25:50 +0200 Subject: [PATCH 1/6] fix(emqx_gateway): return 404 for unknown listener id --- apps/emqx_gateway/src/emqx_gateway_api_listeners.erl | 9 +++++++-- apps/emqx_gateway/src/emqx_gateway_http.erl | 2 +- apps/emqx_gateway/test/emqx_gateway_api_SUITE.erl | 11 +++++++++++ 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/apps/emqx_gateway/src/emqx_gateway_api_listeners.erl b/apps/emqx_gateway/src/emqx_gateway_api_listeners.erl index e62923bc2..046e23300 100644 --- a/apps/emqx_gateway/src/emqx_gateway_api_listeners.erl +++ b/apps/emqx_gateway/src/emqx_gateway_api_listeners.erl @@ -112,8 +112,13 @@ listeners(post, #{bindings := #{name := Name0}, body := LConf}) -> listeners_insta(delete, #{bindings := #{name := Name0, id := ListenerId}}) -> with_gateway(Name0, fun(_GwName, _) -> - ok = emqx_gateway_http:remove_listener(ListenerId), - {204} + case emqx_gateway_conf:listener(ListenerId) of + {ok, _Listener} -> + ok = emqx_gateway_http:remove_listener(ListenerId), + {204}; + {error, not_found} -> + return_http_error(404, "Listener not found") + end end); listeners_insta(get, #{bindings := #{name := Name0, id := ListenerId}}) -> with_gateway(Name0, fun(_GwName, _) -> diff --git a/apps/emqx_gateway/src/emqx_gateway_http.erl b/apps/emqx_gateway/src/emqx_gateway_http.erl index 58c201c75..2186ac3d7 100644 --- a/apps/emqx_gateway/src/emqx_gateway_http.erl +++ b/apps/emqx_gateway/src/emqx_gateway_http.erl @@ -550,7 +550,7 @@ with_gateway(GwName0, Fun) -> return_http_error(400, [K, " is required"]); %% Exceptions from emqx_gateway_utils:parse_listener_id/1 error:{invalid_listener_id, Id} -> - return_http_error(400, ["Invalid listener id: ", Id]); + return_http_error(404, ["Listener not found: ", Id]); %% Exceptions from emqx:get_config/1 error:{config_not_found, Path0} -> Path = lists:concat( diff --git a/apps/emqx_gateway/test/emqx_gateway_api_SUITE.erl b/apps/emqx_gateway/test/emqx_gateway_api_SUITE.erl index f1cfd26d0..e486c8c16 100644 --- a/apps/emqx_gateway/test/emqx_gateway_api_SUITE.erl +++ b/apps/emqx_gateway/test/emqx_gateway_api_SUITE.erl @@ -409,6 +409,7 @@ t_listeners_tcp(_) -> {204, _} = request(delete, "/gateways/stomp/listeners/stomp:tcp:def"), {404, _} = request(get, "/gateways/stomp/listeners/stomp:tcp:def"), + {404, _} = request(delete, "/gateways/stomp/listeners/stomp:tcp:def"), ok. t_listeners_max_conns(_) -> @@ -480,9 +481,19 @@ t_listeners_authn(_) -> {200, ConfResp3} = request(get, Path), assert_confs(AuthConf2, ConfResp3), + {404, _} = request(get, Path ++ "/users/not_exists"), + {404, _} = request(delete, Path ++ "/users/not_exists"), + {204, _} = request(delete, Path), %% FIXME: 204? {204, _} = request(get, Path), + + BadPath = "/gateways/stomp/listeners/stomp:tcp:not_exists/authentication/users/foo", + {404, _} = request(get, BadPath), + {404, _} = request(delete, BadPath), + + {404, _} = request(get, "/gateways/stomp/listeners/not_exists"), + {404, _} = request(delete, "/gateways/stomp/listeners/not_exists"), ok. t_listeners_authn_data_mgmt(_) -> From d65d690c175fe3e8cdc11a2d6eaa53c392654bb4 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Thu, 6 Jul 2023 15:26:15 +0200 Subject: [PATCH 2/6] 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. From 3fd28f9e182ff34d96d85e5f01c6885ab06e562e Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Fri, 7 Jul 2023 11:57:33 +0200 Subject: [PATCH 3/6] 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) -> From 80e4ffff752918fa4db08091a7eb8b2bdf27b576 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Fri, 7 Jul 2023 13:17:52 +0200 Subject: [PATCH 4/6] fix(emqx_management): return 404 if plugin does not exist --- apps/emqx_management/src/emqx_mgmt_api_plugins.erl | 3 +++ apps/emqx_management/test/emqx_mgmt_api_plugins_SUITE.erl | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/apps/emqx_management/src/emqx_mgmt_api_plugins.erl b/apps/emqx_management/src/emqx_mgmt_api_plugins.erl index f50e44771..3db0c42fb 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_plugins.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_plugins.erl @@ -132,6 +132,7 @@ schema("/plugins/:name") -> parameters => [hoconsc:ref(name)], responses => #{ 204 => <<"Uninstall successfully">>, + 400 => emqx_dashboard_swagger:error_codes(['PARAM_ERROR'], <<"Bad parameter">>), 404 => emqx_dashboard_swagger:error_codes(['NOT_FOUND'], <<"Plugin Not Found">>) } } @@ -484,6 +485,8 @@ ensure_action(Name, restart) -> return(Code, ok) -> {Code}; +return(_, {error, #{error := "bad_info_file", return := {enoent, _} = Reason}}) -> + {404, #{code => 'NOT_FOUND', message => iolist_to_binary(io_lib:format("~p", [Reason]))}}; return(_, {error, Reason}) -> {400, #{code => 'PARAM_ERROR', message => iolist_to_binary(io_lib:format("~p", [Reason]))}}. diff --git a/apps/emqx_management/test/emqx_mgmt_api_plugins_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_api_plugins_SUITE.erl index 62fed8211..ba613abc4 100644 --- a/apps/emqx_management/test/emqx_mgmt_api_plugins_SUITE.erl +++ b/apps/emqx_management/test/emqx_mgmt_api_plugins_SUITE.erl @@ -133,6 +133,14 @@ t_bad_plugin(Config) -> ) ). +t_delete_non_existing(_Config) -> + Path = emqx_mgmt_api_test_util:api_path(["plugins", "non_exists"]), + ?assertMatch( + {error, {_, 404, _}}, + emqx_mgmt_api_test_util:request_api(delete, Path) + ), + ok. + list_plugins() -> Path = emqx_mgmt_api_test_util:api_path(["plugins"]), case emqx_mgmt_api_test_util:request_api(get, Path) of From 1110b5d8f5768e314ffcb1570b7d8beb48028f60 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Fri, 7 Jul 2023 13:31:57 +0200 Subject: [PATCH 5/6] fix(emqx_retainer): return 404 in delete if topic not found --- apps/emqx_retainer/src/emqx_retainer.app.src | 2 +- apps/emqx_retainer/src/emqx_retainer_api.erl | 13 +++++++++++-- apps/emqx_retainer/test/emqx_retainer_api_SUITE.erl | 1 + 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/apps/emqx_retainer/src/emqx_retainer.app.src b/apps/emqx_retainer/src/emqx_retainer.app.src index d13359509..f117fda05 100644 --- a/apps/emqx_retainer/src/emqx_retainer.app.src +++ b/apps/emqx_retainer/src/emqx_retainer.app.src @@ -2,7 +2,7 @@ {application, emqx_retainer, [ {description, "EMQX Retainer"}, % strict semver, bump manually! - {vsn, "5.0.14"}, + {vsn, "5.0.15"}, {modules, []}, {registered, [emqx_retainer_sup]}, {applications, [kernel, stdlib, emqx, emqx_ctl]}, diff --git a/apps/emqx_retainer/src/emqx_retainer_api.erl b/apps/emqx_retainer/src/emqx_retainer_api.erl index 7b1337140..3274f0e4c 100644 --- a/apps/emqx_retainer/src/emqx_retainer_api.erl +++ b/apps/emqx_retainer/src/emqx_retainer_api.erl @@ -102,6 +102,7 @@ schema(?PREFIX ++ "/message/:topic") -> parameters => parameters(), responses => #{ 204 => <<>>, + 404 => error_codes(['NOT_FOUND'], ?DESC(message_not_exist)), 400 => error_codes( ['BAD_REQUEST'], ?DESC(unsupported_backend) @@ -187,8 +188,16 @@ with_topic(get, #{bindings := Bindings}) -> end; with_topic(delete, #{bindings := Bindings}) -> Topic = maps:get(topic, Bindings), - emqx_retainer_mnesia:delete_message(undefined, Topic), - {204}. + case emqx_retainer_mnesia:page_read(undefined, Topic, 1, 1) of + {ok, []} -> + {404, #{ + code => <<"NOT_FOUND">>, + message => <<"Viewed message doesn't exist">> + }}; + {ok, _} -> + emqx_retainer_mnesia:delete_message(undefined, Topic), + {204} + end. format_message(#message{ id = ID, diff --git a/apps/emqx_retainer/test/emqx_retainer_api_SUITE.erl b/apps/emqx_retainer/test/emqx_retainer_api_SUITE.erl index 61eee0510..d00ade556 100644 --- a/apps/emqx_retainer/test/emqx_retainer_api_SUITE.erl +++ b/apps/emqx_retainer/test/emqx_retainer_api_SUITE.erl @@ -218,6 +218,7 @@ t_lookup_and_delete(_) -> {ok, []} = request_api(delete, API), {error, {"HTTP/1.1", 404, "Not Found"}} = request_api(get, API), + {error, {"HTTP/1.1", 404, "Not Found"}} = request_api(delete, API), ok = emqtt:disconnect(C1). From da052e0a5e1720c63930232701f47477ac5f1e09 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Fri, 7 Jul 2023 16:38:27 +0200 Subject: [PATCH 6/6] chore: add changelog --- changes/ce/fix-11211.en.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changes/ce/fix-11211.en.md diff --git a/changes/ce/fix-11211.en.md b/changes/ce/fix-11211.en.md new file mode 100644 index 000000000..0b69fefca --- /dev/null +++ b/changes/ce/fix-11211.en.md @@ -0,0 +1 @@ +Consistently return `404` for `DELETE` operations on non-existent resources.