Merge pull request #10085 from sstrigler/EMQX-8552-authorization-sources-type-status-move-shouldnt-exist-when-authorization-sources-type-doesnt-exist

emqx_authz API: return 404 for all requests on non existent source
2023-03-07 16:19:28 +01:00 · 2023-03-07 16:19:28 +01:00 · 609cd01a35
parent 0a5b221984 71cb3be1d0
commit 609cd01a35
4 changed files with 193 additions and 115 deletions
--- 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).
--- 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(
@ -371,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(
@ -382,7 +420,6 @@ t_api(_) ->
        ]
    ),
    emqtt:connect(Client),
-    timer:sleep(50),

    emqtt:publish(
        Client,
@ -392,17 +429,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 +455,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,20 +481,26 @@ 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(_) ->
+t_source_move(_) ->
    {ok, _} = emqx_authz:update(replace, [?SOURCE1, ?SOURCE2, ?SOURCE3, ?SOURCE4, ?SOURCE5]),
    ?assertMatch(
        [
@ -564,7 +621,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().

--- a/changes/ce/fix-10085.en.md
+++ b/changes/ce/fix-10085.en.md
@ -0,0 +1 @@
+Consistently return `404` for all requests on non existent source in `/authorization/sources/:source[/*]`.
--- a/changes/ce/fix-10085.zh.md
+++ b/changes/ce/fix-10085.zh.md
@ -0,0 +1 @@
+如果向 `/authorization/sources/:source[/*]`  请求的 `source` 不存在，将一致地返回 `404`。
				`@ -0,0 +1 @@`
				Consistently return `404` for all requests on non existent source in `/authorization/sources/:source[/*]`.
				`@ -0,0 +1 @@`
				如果向 `/authorization/sources/:source[/*]` 请求的 `source` 不存在，将一致地返回 `404`。