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).