From 36b1c3eb5be81a94ae83bb65fff5d6cdcab3a1b0 Mon Sep 17 00:00:00 2001 From: JianBo He Date: Mon, 13 Feb 2023 11:58:13 +0800 Subject: [PATCH 1/3] fix(api): fix bad return error message if client id is not found --- .../src/emqx_mgmt_api_clients.erl | 54 ++++++++++--------- .../test/emqx_mgmt_api_clients_SUITE.erl | 43 +++++++++++++++ 2 files changed, 71 insertions(+), 26 deletions(-) diff --git a/apps/emqx_management/src/emqx_mgmt_api_clients.erl b/apps/emqx_management/src/emqx_mgmt_api_clients.erl index 7c45206fd..571f190f2 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_clients.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_clients.erl @@ -76,9 +76,10 @@ -define(FORMAT_FUN, {?MODULE, format_channel_info}). --define(CLIENT_ID_NOT_FOUND, - <<"{\"code\": \"RESOURCE_NOT_FOUND\", \"reason\": \"Client id not found\"}">> -). +-define(CLIENTID_NOT_FOUND, #{ + code => 'CLIENTID_NOT_FOUND', + message => <<"Client ID not found">> +}). api_spec() -> emqx_dashboard_swagger:spec(?MODULE, #{check_schema => true, translate_body => true}). @@ -219,7 +220,7 @@ schema("/clients/:clientid") -> responses => #{ 200 => hoconsc:mk(hoconsc:ref(?MODULE, client), #{}), 404 => emqx_dashboard_swagger:error_codes( - ['CLIENTID_NOT_FOUND'], <<"Client id not found">> + ['CLIENTID_NOT_FOUND'], <<"Client ID not found">> ) } }, @@ -232,7 +233,7 @@ schema("/clients/:clientid") -> responses => #{ 204 => <<"Kick out client successfully">>, 404 => emqx_dashboard_swagger:error_codes( - ['CLIENTID_NOT_FOUND'], <<"Client id not found">> + ['CLIENTID_NOT_FOUND'], <<"Client ID not found">> ) } } @@ -247,7 +248,7 @@ schema("/clients/:clientid/authorization/cache") -> responses => #{ 200 => hoconsc:mk(hoconsc:ref(?MODULE, authz_cache), #{}), 404 => emqx_dashboard_swagger:error_codes( - ['CLIENTID_NOT_FOUND'], <<"Client id not found">> + ['CLIENTID_NOT_FOUND'], <<"Client ID not found">> ) } }, @@ -256,9 +257,9 @@ schema("/clients/:clientid/authorization/cache") -> tags => ?TAGS, parameters => [{clientid, hoconsc:mk(binary(), #{in => path})}], responses => #{ - 204 => <<"Kick out client successfully">>, + 204 => <<"Clean client authz cache successfully">>, 404 => emqx_dashboard_swagger:error_codes( - ['CLIENTID_NOT_FOUND'], <<"Client id not found">> + ['CLIENTID_NOT_FOUND'], <<"Client ID not found">> ) } } @@ -273,10 +274,11 @@ 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">> + %) } } }; @@ -291,7 +293,7 @@ schema("/clients/:clientid/subscribe") -> responses => #{ 200 => hoconsc:ref(emqx_mgmt_api_subscriptions, subscription), 404 => emqx_dashboard_swagger:error_codes( - ['CLIENTID_NOT_FOUND'], <<"Client id not found">> + ['CLIENTID_NOT_FOUND'], <<"Client ID not found">> ) } } @@ -307,7 +309,7 @@ schema("/clients/:clientid/subscribe/bulk") -> responses => #{ 200 => hoconsc:array(hoconsc:ref(emqx_mgmt_api_subscriptions, subscription)), 404 => emqx_dashboard_swagger:error_codes( - ['CLIENTID_NOT_FOUND'], <<"Client id not found">> + ['CLIENTID_NOT_FOUND'], <<"Client ID not found">> ) } } @@ -323,7 +325,7 @@ schema("/clients/:clientid/unsubscribe") -> responses => #{ 204 => <<"Unsubscribe OK">>, 404 => emqx_dashboard_swagger:error_codes( - ['CLIENTID_NOT_FOUND'], <<"Client id not found">> + ['CLIENTID_NOT_FOUND'], <<"Client ID not found">> ) } } @@ -339,7 +341,7 @@ schema("/clients/:clientid/unsubscribe/bulk") -> responses => #{ 204 => <<"Unsubscribe OK">>, 404 => emqx_dashboard_swagger:error_codes( - ['CLIENTID_NOT_FOUND'], <<"Client id not found">> + ['CLIENTID_NOT_FOUND'], <<"Client ID not found">> ) } } @@ -355,7 +357,7 @@ schema("/clients/:clientid/keepalive") -> responses => #{ 200 => hoconsc:mk(hoconsc:ref(?MODULE, client), #{}), 404 => emqx_dashboard_swagger:error_codes( - ['CLIENTID_NOT_FOUND'], <<"Client id not found">> + ['CLIENTID_NOT_FOUND'], <<"Client ID not found">> ) } } @@ -621,7 +623,7 @@ set_keepalive(put, #{bindings := #{clientid := ClientID}, body := Body}) -> {ok, Interval} -> case emqx_mgmt:set_keepalive(emqx_mgmt_util:urldecode(ClientID), Interval) of ok -> lookup(#{clientid => ClientID}); - {error, not_found} -> {404, ?CLIENT_ID_NOT_FOUND}; + {error, not_found} -> {404, ?CLIENTID_NOT_FOUND}; {error, Reason} -> {400, #{code => 'PARAMS_ERROR', message => Reason}} end end. @@ -669,7 +671,7 @@ list_clients(QString) -> lookup(#{clientid := ClientID}) -> case emqx_mgmt:lookup_client({clientid, ClientID}, ?FORMAT_FUN) of [] -> - {404, ?CLIENT_ID_NOT_FOUND}; + {404, ?CLIENTID_NOT_FOUND}; ClientInfo -> {200, hd(ClientInfo)} end. @@ -677,7 +679,7 @@ lookup(#{clientid := ClientID}) -> kickout(#{clientid := ClientID}) -> case emqx_mgmt:kickout_client({ClientID, ?FORMAT_FUN}) of {error, not_found} -> - {404, ?CLIENT_ID_NOT_FOUND}; + {404, ?CLIENTID_NOT_FOUND}; _ -> {204} end. @@ -685,7 +687,7 @@ kickout(#{clientid := ClientID}) -> get_authz_cache(#{clientid := ClientID}) -> case emqx_mgmt:list_authz_cache(ClientID) of {error, not_found} -> - {404, ?CLIENT_ID_NOT_FOUND}; + {404, ?CLIENTID_NOT_FOUND}; {error, Reason} -> Message = list_to_binary(io_lib:format("~p", [Reason])), {500, #{code => <<"UNKNOW_ERROR">>, message => Message}}; @@ -699,7 +701,7 @@ clean_authz_cache(#{clientid := ClientID}) -> ok -> {204}; {error, not_found} -> - {404, ?CLIENT_ID_NOT_FOUND}; + {404, ?CLIENTID_NOT_FOUND}; {error, Reason} -> Message = list_to_binary(io_lib:format("~p", [Reason])), {500, #{code => <<"UNKNOW_ERROR">>, message => Message}} @@ -709,7 +711,7 @@ subscribe(#{clientid := ClientID, topic := Topic} = Sub) -> Opts = maps:with([qos, nl, rap, rh], Sub), case do_subscribe(ClientID, Topic, Opts) of {error, channel_not_found} -> - {404, ?CLIENT_ID_NOT_FOUND}; + {404, ?CLIENTID_NOT_FOUND}; {error, Reason} -> Message = list_to_binary(io_lib:format("~p", [Reason])), {500, #{code => <<"UNKNOW_ERROR">>, message => Message}}; @@ -723,7 +725,7 @@ subscribe_batch(#{clientid := ClientID, topics := Topics}) -> %% has returned. So if one want to subscribe topics in this hook, it will fail. case ets:lookup(emqx_channel, ClientID) of [] -> - {404, ?CLIENT_ID_NOT_FOUND}; + {404, ?CLIENTID_NOT_FOUND}; _ -> ArgList = [ [ClientID, Topic, maps:with([qos, nl, rap, rh], Sub)] @@ -735,7 +737,7 @@ subscribe_batch(#{clientid := ClientID, topics := Topics}) -> unsubscribe(#{clientid := ClientID, topic := Topic}) -> case do_unsubscribe(ClientID, Topic) of {error, channel_not_found} -> - {404, ?CLIENT_ID_NOT_FOUND}; + {404, ?CLIENTID_NOT_FOUND}; {unsubscribe, [{Topic, #{}}]} -> {204} end. @@ -745,8 +747,8 @@ unsubscribe_batch(#{clientid := ClientID, topics := Topics}) -> {200, _} -> _ = emqx_mgmt:unsubscribe_batch(ClientID, Topics), {204}; - {404, ?CLIENT_ID_NOT_FOUND} -> - {404, ?CLIENT_ID_NOT_FOUND} + {404, NotFound} -> + {404, NotFound} end. %%-------------------------------------------------------------------- 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 16ba99ad6..1a74d3af6 100644 --- a/apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl +++ b/apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl @@ -247,6 +247,49 @@ t_keepalive(_Config) -> emqtt:disconnect(C1), ok. +t_client_id_not_found(_Config) -> + AuthHeader = emqx_mgmt_api_test_util:auth_header_(), + Http = {"HTTP/1.1", 404, "Not Found"}, + Body = "{\"code\":\"CLIENTID_NOT_FOUND\",\"message\":\"Client ID not found\"}", + + PathFun = fun(Suffix) -> + emqx_mgmt_api_test_util:api_path(["clients", "no_existed_clientid"] ++ Suffix) + end, + ReqFun = fun(Method, Path) -> + emqx_mgmt_api_test_util:request_api( + Method, Path, "", AuthHeader, [], #{return_all => true} + ) + end, + + PostFun = fun(Method, Path, Data) -> + emqx_mgmt_api_test_util:request_api( + Method, Path, "", AuthHeader, Data, #{return_all => true} + ) + end, + + %% Client lookup + ?assertMatch({error, {Http, _, Body}}, ReqFun(get, PathFun([]))), + %% Client kickout + ?assertMatch({error, {Http, _, Body}}, ReqFun(delete, PathFun([]))), + %% Client Subscription list + ?assertMatch({ok, {{"HTTP/1.1", 200, "OK"}, _, "[]"}}, ReqFun(get, PathFun(["subscriptions"]))), + %% AuthZ Cache lookup + ?assertMatch({error, {Http, _, Body}}, ReqFun(get, PathFun(["authorization", "cache"]))), + %% AuthZ Cache clean + ?assertMatch({error, {Http, _, Body}}, ReqFun(delete, PathFun(["authorization", "cache"]))), + %% Client Subscribe + SubBody = #{topic => <<"testtopic">>, qos => 1, nl => 1, rh => 1}, + ?assertMatch({error, {Http, _, Body}}, PostFun(post, PathFun(["subscribe"]), SubBody)), + ?assertMatch( + {error, {Http, _, Body}}, PostFun(post, PathFun(["subscribe", "bulk"]), [SubBody]) + ), + %% Client Unsubscribe + UnsubBody = #{topic => <<"testtopic">>}, + ?assertMatch({error, {Http, _, Body}}, PostFun(post, PathFun(["unsubscribe"]), UnsubBody)), + ?assertMatch( + {error, {Http, _, Body}}, PostFun(post, PathFun(["unsubscribe", "bulk"]), [UnsubBody]) + ). + time_string_to_epoch_millisecond(DateTime) -> time_string_to_epoch(DateTime, millisecond). From a9b0885af0fcea6b45af6972a88c33bbd655875f Mon Sep 17 00:00:00 2001 From: JianBo He Date: Mon, 13 Feb 2023 13:54:25 +0800 Subject: [PATCH 2/3] chore: update changes --- changes/v5.0.17/fix-9958.en.md | 1 + changes/v5.0.17/fix-9958.zh.md | 1 + 2 files changed, 2 insertions(+) create mode 100644 changes/v5.0.17/fix-9958.en.md create mode 100644 changes/v5.0.17/fix-9958.zh.md diff --git a/changes/v5.0.17/fix-9958.en.md b/changes/v5.0.17/fix-9958.en.md new file mode 100644 index 000000000..821934ad0 --- /dev/null +++ b/changes/v5.0.17/fix-9958.en.md @@ -0,0 +1 @@ +Fix bad http response format when client ID is not found in `clients` APIs diff --git a/changes/v5.0.17/fix-9958.zh.md b/changes/v5.0.17/fix-9958.zh.md new file mode 100644 index 000000000..a26fbb7fe --- /dev/null +++ b/changes/v5.0.17/fix-9958.zh.md @@ -0,0 +1 @@ +修复 `clients` API 在 Client ID 不存在时返回的错误的 HTTP 应答格式。 From 0d63cfdc9793f5946c647bcc110a91b812e1c280 Mon Sep 17 00:00:00 2001 From: JianBo He Date: Tue, 14 Feb 2023 15:31:42 +0800 Subject: [PATCH 3/3] chore: remove changes to 5.0.18 --- apps/emqx_management/src/emqx_management.app.src | 2 +- changes/{v5.0.17 => v5.0.18}/fix-9958.en.md | 0 changes/{v5.0.17 => v5.0.18}/fix-9958.zh.md | 0 3 files changed, 1 insertion(+), 1 deletion(-) rename changes/{v5.0.17 => v5.0.18}/fix-9958.en.md (100%) rename changes/{v5.0.17 => v5.0.18}/fix-9958.zh.md (100%) diff --git a/apps/emqx_management/src/emqx_management.app.src b/apps/emqx_management/src/emqx_management.app.src index fdd2af9b2..8a10f1b6b 100644 --- a/apps/emqx_management/src/emqx_management.app.src +++ b/apps/emqx_management/src/emqx_management.app.src @@ -2,7 +2,7 @@ {application, emqx_management, [ {description, "EMQX Management API and CLI"}, % strict semver, bump manually! - {vsn, "5.0.14"}, + {vsn, "5.0.15"}, {modules, []}, {registered, [emqx_management_sup]}, {applications, [kernel, stdlib, emqx_plugins, minirest, emqx]}, diff --git a/changes/v5.0.17/fix-9958.en.md b/changes/v5.0.18/fix-9958.en.md similarity index 100% rename from changes/v5.0.17/fix-9958.en.md rename to changes/v5.0.18/fix-9958.en.md diff --git a/changes/v5.0.17/fix-9958.zh.md b/changes/v5.0.18/fix-9958.zh.md similarity index 100% rename from changes/v5.0.17/fix-9958.zh.md rename to changes/v5.0.18/fix-9958.zh.md