From a7605fba946c06e610071d48f5976828b42765c3 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Tue, 7 Mar 2023 11:32:03 +0100 Subject: [PATCH 1/4] test(emqx_authz): use snabbkaffe:retry instead of timer:sleep also use emqx_json rather than jiffy or jsx directly --- .../test/emqx_authz_api_sources_SUITE.erl | 102 +++++++++++------- 1 file changed, 61 insertions(+), 41 deletions(-) diff --git a/apps/emqx_authz/test/emqx_authz_api_sources_SUITE.erl b/apps/emqx_authz/test/emqx_authz_api_sources_SUITE.erl index f2761412a..cf7039133 100644 --- a/apps/emqx_authz/test/emqx_authz_api_sources_SUITE.erl +++ b/apps/emqx_authz/test/emqx_authz_api_sources_SUITE.erl @@ -21,7 +21,6 @@ -import(emqx_mgmt_api_test_util, [request/3, uri/1]). -include_lib("eunit/include/eunit.hrl"). --include_lib("common_test/include/ct.hrl"). -include_lib("emqx/include/emqx_placeholder.hrl"). -define(MONGO_SINGLE_HOST, "mongo"). @@ -183,7 +182,7 @@ t_api(_) -> {ok, 404, ErrResult} = request(get, uri(["authorization", "sources", "http"]), []), ?assertMatch( #{<<"code">> := <<"NOT_FOUND">>, <<"message">> := <<"Not found: http">>}, - jsx:decode(ErrResult) + emqx_json:decode(ErrResult, [return_maps]) ), [ @@ -215,7 +214,9 @@ t_api(_) -> ?SOURCE1#{<<"enable">> := false} ), {ok, 200, Result3} = request(get, uri(["authorization", "sources", "http"]), []), - ?assertMatch(#{<<"type">> := <<"http">>, <<"enable">> := false}, jsx:decode(Result3)), + ?assertMatch( + #{<<"type">> := <<"http">>, <<"enable">> := false}, emqx_json:decode(Result3, [return_maps]) + ), Keyfile = emqx_common_test_helpers:app_path( emqx, @@ -252,7 +253,7 @@ t_api(_) -> <<"total">> := 0, <<"nomatch">> := 0 } - } = jiffy:decode(Status4, [return_maps]), + } = emqx_json:decode(Status4, [return_maps]), ?assertMatch( #{ <<"type">> := <<"mongodb">>, @@ -264,7 +265,7 @@ t_api(_) -> <<"verify">> := <<"verify_none">> } }, - jsx:decode(Result4) + emqx_json:decode(Result4, [return_maps]) ), {ok, Cacert} = file:read_file(Cacertfile), @@ -296,7 +297,7 @@ t_api(_) -> <<"verify">> := <<"verify_none">> } }, - jsx:decode(Result5) + emqx_json:decode(Result5, [return_maps]) ), {ok, 200, Status5_1} = request(get, uri(["authorization", "sources", "mongodb", "status"]), []), @@ -307,7 +308,7 @@ t_api(_) -> <<"total">> := 0, <<"nomatch">> := 0 } - } = jiffy:decode(Status5_1, [return_maps]), + } = emqx_json:decode(Status5_1, [return_maps]), #{ ssl := #{ @@ -354,7 +355,7 @@ t_api(_) -> <<"code">> := <<"BAD_REQUEST">>, <<"message">> := <<"Type mismatch", _/binary>> }, - jiffy:decode(TypeMismatch, [return_maps]) + emqx_json:decode(TypeMismatch, [return_maps]) ), lists:foreach( @@ -382,7 +383,6 @@ t_api(_) -> ] ), emqtt:connect(Client), - timer:sleep(50), emqtt:publish( Client, @@ -392,17 +392,24 @@ t_api(_) -> [{qos, 1}] ), - {ok, 200, Status5} = request(get, uri(["authorization", "sources", "file", "status"]), []), - #{ - <<"metrics">> := #{ - <<"allow">> := 1, - <<"deny">> := 0, - <<"total">> := 1, - <<"nomatch">> := 0 - } - } = jiffy:decode(Status5, [return_maps]), + snabbkaffe:retry( + 10, + 3, + fun() -> + {ok, 200, Status5} = request( + get, uri(["authorization", "sources", "file", "status"]), [] + ), + #{ + <<"metrics">> := #{ + <<"allow">> := 1, + <<"deny">> := 0, + <<"total">> := 1, + <<"nomatch">> := 0 + } + } = emqx_json:decode(Status5, [return_maps]) + end + ), - timer:sleep(50), emqtt:publish( Client, <<"t2">>, @@ -411,17 +418,24 @@ t_api(_) -> [{qos, 1}] ), - {ok, 200, Status6} = request(get, uri(["authorization", "sources", "file", "status"]), []), - #{ - <<"metrics">> := #{ - <<"allow">> := 2, - <<"deny">> := 0, - <<"total">> := 2, - <<"nomatch">> := 0 - } - } = jiffy:decode(Status6, [return_maps]), + snabbkaffe:retry( + 10, + 3, + fun() -> + {ok, 200, Status6} = request( + get, uri(["authorization", "sources", "file", "status"]), [] + ), + #{ + <<"metrics">> := #{ + <<"allow">> := 2, + <<"deny">> := 0, + <<"total">> := 2, + <<"nomatch">> := 0 + } + } = emqx_json:decode(Status6, [return_maps]) + end + ), - timer:sleep(50), emqtt:publish( Client, <<"t3">>, @@ -430,17 +444,23 @@ t_api(_) -> [{qos, 1}] ), - timer:sleep(50), - {ok, 200, Status7} = request(get, uri(["authorization", "sources", "file", "status"]), []), - #{ - <<"metrics">> := #{ - <<"allow">> := 3, - <<"deny">> := 0, - <<"total">> := 3, - <<"nomatch">> := 0 - } - } = jiffy:decode(Status7, [return_maps]), - + snabbkaffe:retry( + 10, + 3, + fun() -> + {ok, 200, Status7} = request( + get, uri(["authorization", "sources", "file", "status"]), [] + ), + #{ + <<"metrics">> := #{ + <<"allow">> := 3, + <<"deny">> := 0, + <<"total">> := 3, + <<"nomatch">> := 0 + } + } = emqx_json:decode(Status7, [return_maps]) + end + ), ok. t_move_source(_) -> @@ -564,7 +584,7 @@ t_aggregate_metrics(_) -> ). get_sources(Result) -> - maps:get(<<"sources">>, jsx:decode(Result), []). + maps:get(<<"sources">>, emqx_json:decode(Result, [return_maps])). data_dir() -> emqx:data_dir(). From b54f444263b8dfa0e8c32aecfdba62ff39958950 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Tue, 7 Mar 2023 13:48:22 +0100 Subject: [PATCH 2/4] fix(emqx_authz): return `404` for requests on non existent source --- .../emqx_authz/src/emqx_authz_api_sources.erl | 165 ++++++++++-------- .../test/emqx_authz_api_sources_SUITE.erl | 39 ++++- 2 files changed, 130 insertions(+), 74 deletions(-) diff --git a/apps/emqx_authz/src/emqx_authz_api_sources.erl b/apps/emqx_authz/src/emqx_authz_api_sources.erl index fffb4bee4..f3b8d0f48 100644 --- a/apps/emqx_authz/src/emqx_authz_api_sources.erl +++ b/apps/emqx_authz/src/emqx_authz_api_sources.erl @@ -47,7 +47,7 @@ -export([ sources/2, source/2, - move_source/2, + source_move/2, aggregate_metrics/1 ]). @@ -164,7 +164,7 @@ schema("/authorization/sources/:type/status") -> }; schema("/authorization/sources/:type/move") -> #{ - 'operationId' => move_source, + 'operationId' => source_move, post => #{ description => ?DESC(authorization_sources_type_move_post), @@ -230,8 +230,6 @@ sources(get, _) -> get_raw_sources() ), {200, #{sources => Sources}}; -sources(post, #{body := #{<<"type">> := <<"file">>} = Body}) -> - create_authz_file(Body); sources(post, #{body := Body}) -> update_config(?CMD_PREPEND, Body). @@ -240,77 +238,99 @@ source(Method, #{bindings := #{type := Type} = Bindings} = Req) when -> source(Method, Req#{bindings => Bindings#{type => atom_to_binary(Type, utf8)}}); source(get, #{bindings := #{type := Type}}) -> - case get_raw_source(Type) of - [] -> - {404, #{code => <<"NOT_FOUND">>, message => <<"Not found: ", Type/binary>>}}; - [#{<<"type">> := <<"file">>, <<"enable">> := Enable, <<"path">> := Path}] -> - case file:read_file(Path) of - {ok, Rules} -> - {200, #{ - type => file, - enable => Enable, - rules => Rules - }}; - {error, Reason} -> - {500, #{ - code => <<"INTERNAL_ERROR">>, - message => bin(Reason) - }} - end; - [Source] -> - {200, Source} - end; -source(put, #{bindings := #{type := <<"file">>}, body := #{<<"type">> := <<"file">>} = Body}) -> - update_authz_file(Body); + with_source( + Type, + fun + (#{<<"type">> := <<"file">>, <<"enable">> := Enable, <<"path">> := Path}) -> + case file:read_file(Path) of + {ok, Rules} -> + {200, #{ + type => file, + enable => Enable, + rules => Rules + }}; + {error, Reason} -> + {500, #{ + code => <<"INTERNAL_ERROR">>, + message => bin(Reason) + }} + end; + (Source) -> + {200, Source} + end + ); source(put, #{bindings := #{type := Type}, body := #{<<"type">> := Type} = Body}) -> - update_config({?CMD_REPLACE, Type}, Body); -source(put, #{bindings := #{type := _Type}, body := #{<<"type">> := _OtherType}}) -> - {400, #{code => <<"BAD_REQUEST">>, message => <<"Type mismatch">>}}; + with_source( + Type, + fun(_) -> + update_config({?CMD_REPLACE, Type}, Body) + end + ); +source(put, #{bindings := #{type := Type}, body := #{<<"type">> := _OtherType}}) -> + with_source( + Type, + fun(_) -> + {400, #{code => <<"BAD_REQUEST">>, message => <<"Type mismatch">>}} + end + ); source(delete, #{bindings := #{type := Type}}) -> - update_config({?CMD_DELETE, Type}, #{}). + with_source( + Type, + fun(_) -> + update_config({?CMD_DELETE, Type}, #{}) + end + ). source_status(get, #{bindings := #{type := Type}}) -> - lookup_from_all_nodes(Type). + with_source( + atom_to_binary(Type, utf8), + fun(_) -> lookup_from_all_nodes(Type) end + ). -move_source(Method, #{bindings := #{type := Type} = Bindings} = Req) when +source_move(Method, #{bindings := #{type := Type} = Bindings} = Req) when is_atom(Type) -> - move_source(Method, Req#{bindings => Bindings#{type => atom_to_binary(Type, utf8)}}); -move_source(post, #{bindings := #{type := Type}, body := #{<<"position">> := Position}}) -> - case parse_position(Position) of - {ok, NPosition} -> - try emqx_authz:move(Type, NPosition) of - {ok, _} -> - {204}; - {error, {not_found_source, _Type}} -> - {404, #{ - code => <<"NOT_FOUND">>, - message => <<"source ", Type/binary, " not found">> - }}; - {error, {emqx_conf_schema, _}} -> - {400, #{ - code => <<"BAD_REQUEST">>, - message => <<"BAD_SCHEMA">> - }}; + source_move(Method, Req#{bindings => Bindings#{type => atom_to_binary(Type, utf8)}}); +source_move(post, #{bindings := #{type := Type}, body := #{<<"position">> := Position}}) -> + with_source( + Type, + fun(_Source) -> + case parse_position(Position) of + {ok, NPosition} -> + try emqx_authz:move(Type, NPosition) of + {ok, _} -> + {204}; + {error, {not_found_source, _Type}} -> + {404, #{ + code => <<"NOT_FOUND">>, + message => <<"source ", Type/binary, " not found">> + }}; + {error, {emqx_conf_schema, _}} -> + {400, #{ + code => <<"BAD_REQUEST">>, + message => <<"BAD_SCHEMA">> + }}; + {error, Reason} -> + {400, #{ + code => <<"BAD_REQUEST">>, + message => bin(Reason) + }} + catch + error:{unknown_authz_source_type, Unknown} -> + NUnknown = bin(Unknown), + {400, #{ + code => <<"BAD_REQUEST">>, + message => <<"Unknown authz Source Type: ", NUnknown/binary>> + }} + end; {error, Reason} -> {400, #{ code => <<"BAD_REQUEST">>, message => bin(Reason) }} - catch - error:{unknown_authz_source_type, Unknown} -> - NUnknown = bin(Unknown), - {400, #{ - code => <<"BAD_REQUEST">>, - message => <<"Unknown authz Source Type: ", NUnknown/binary>> - }} - end; - {error, Reason} -> - {400, #{ - code => <<"BAD_REQUEST">>, - message => bin(Reason) - }} - end. + end + end + ). %%-------------------------------------------------------------------- %% Internal functions @@ -486,6 +506,15 @@ get_raw_source(Type) -> get_raw_sources() ). +-spec with_source(binary(), fun((map()) -> term())) -> term(). +with_source(Type, ContF) -> + case get_raw_source(Type) of + [] -> + {404, #{code => <<"NOT_FOUND">>, message => <<"Not found: ", Type/binary>>}}; + [Source] -> + ContF(Source) + end. + update_config(Cmd, Sources) -> case emqx_authz:update(Cmd, Sources) of {ok, _} -> @@ -630,13 +659,3 @@ status_metrics_example() -> } } }. - -create_authz_file(Body) -> - do_update_authz_file(?CMD_PREPEND, Body). - -update_authz_file(Body) -> - do_update_authz_file({?CMD_REPLACE, <<"file">>}, Body). - -do_update_authz_file(Cmd, Body) -> - %% API update will placed in `authz` subdirectory inside EMQX's `data_dir` - update_config(Cmd, Body). diff --git a/apps/emqx_authz/test/emqx_authz_api_sources_SUITE.erl b/apps/emqx_authz/test/emqx_authz_api_sources_SUITE.erl index cf7039133..411399d64 100644 --- a/apps/emqx_authz/test/emqx_authz_api_sources_SUITE.erl +++ b/apps/emqx_authz/test/emqx_authz_api_sources_SUITE.erl @@ -372,6 +372,43 @@ t_api(_) -> ?assertEqual([], get_sources(Result6)), ?assertEqual([], emqx:get_config([authorization, sources])), + lists:foreach( + fun(#{<<"type">> := Type}) -> + {ok, 404, _} = request( + get, + uri(["authorization", "sources", binary_to_list(Type), "status"]), + [] + ), + {ok, 404, _} = request( + post, + uri(["authorization", "sources", binary_to_list(Type), "move"]), + #{<<"position">> => <<"front">>} + ), + {ok, 404, _} = request( + get, + uri(["authorization", "sources", binary_to_list(Type)]), + [] + ), + {ok, 404, _} = request( + delete, + uri(["authorization", "sources", binary_to_list(Type)]), + [] + ) + end, + Sources + ), + + {ok, 404, _TypeMismatch2} = request( + put, + uri(["authorization", "sources", "file"]), + #{<<"type">> => <<"built_in_database">>, <<"enable">> => false} + ), + {ok, 404, _} = request( + put, + uri(["authorization", "sources", "built_in_database"]), + #{<<"type">> => <<"built_in_database">>, <<"enable">> => false} + ), + {ok, 204, _} = request(post, uri(["authorization", "sources"]), ?SOURCE6), {ok, Client} = emqtt:start_link( @@ -463,7 +500,7 @@ t_api(_) -> ), ok. -t_move_source(_) -> +t_source_move(_) -> {ok, _} = emqx_authz:update(replace, [?SOURCE1, ?SOURCE2, ?SOURCE3, ?SOURCE4, ?SOURCE5]), ?assertMatch( [ From e1054be3194e947049aa51fa1c535a6c6d381494 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Tue, 7 Mar 2023 13:56:22 +0100 Subject: [PATCH 3/4] chore: add changelog --- changes/ce/fix-10085.en.md | 1 + changes/ce/fix-10085.zh.md | 1 + 2 files changed, 2 insertions(+) create mode 100644 changes/ce/fix-10085.en.md create mode 100644 changes/ce/fix-10085.zh.md diff --git a/changes/ce/fix-10085.en.md b/changes/ce/fix-10085.en.md new file mode 100644 index 000000000..e539a04b4 --- /dev/null +++ b/changes/ce/fix-10085.en.md @@ -0,0 +1 @@ +Consistently return `404` for all requests on non existent source in `/authorization/sources/:source[/*]`. diff --git a/changes/ce/fix-10085.zh.md b/changes/ce/fix-10085.zh.md new file mode 100644 index 000000000..3e2d809d4 --- /dev/null +++ b/changes/ce/fix-10085.zh.md @@ -0,0 +1 @@ +对 `/authorization/sources/:source[/*] `中不存在的来源的所有请求一致地返回 `404`。 From 71cb3be1d0176c756925dabac34a0a42053f0369 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Tue, 7 Mar 2023 15:12:51 +0100 Subject: [PATCH 4/4] chore: fix translation --- changes/ce/fix-10085.zh.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/ce/fix-10085.zh.md b/changes/ce/fix-10085.zh.md index 3e2d809d4..059680efa 100644 --- a/changes/ce/fix-10085.zh.md +++ b/changes/ce/fix-10085.zh.md @@ -1 +1 @@ -对 `/authorization/sources/:source[/*] `中不存在的来源的所有请求一致地返回 `404`。 +如果向 `/authorization/sources/:source[/*]` 请求的 `source` 不存在，将一致地返回 `404`。