Merge pull request #9328 from sstrigler/master

fix(emqx_authn_api): return 404 for status of unknown authenticator

apps/emqx/test/emqx_trace_SUITE.erl

@@ -40,7 +40,7 @@ init_per_suite(Config) ->
         ?wait_async_action(
             emqx_common_test_helpers:start_apps([]),
             #{?snk_kind := listener_started, bind := 1883},
-            timer:seconds(10)
+            timer:seconds(100)
         ),
         fun(Trace) ->
             %% more than one listener

apps/emqx_authn/src/emqx_authn.app.src

@@ -1,7 +1,7 @@
 %% -*- mode: erlang -*-
 {application, emqx_authn, [
     {description, "EMQX Authentication"},
-    {vsn, "0.1.8"},
+    {vsn, "0.1.9"},
     {modules, []},
     {registered, [emqx_authn_sup, emqx_authn_registry]},
     {applications, [kernel, stdlib, emqx_resource, ehttpc, epgsql, mysql, jose]},

apps/emqx_authn/src/emqx_authn_api.erl

@@ -30,6 +30,7 @@
 -define(BAD_REQUEST, 'BAD_REQUEST').
 -define(NOT_FOUND, 'NOT_FOUND').
 -define(ALREADY_EXISTS, 'ALREADY_EXISTS').
+-define(INTERNAL_ERROR, 'INTERNAL_ERROR').
 
 % Swagger
 
@@ -224,7 +225,8 @@ schema("/authentication/:id/status") ->
                     hoconsc:ref(emqx_authn_schema, "metrics_status_fields"),
                     status_metrics_example()
                 ),
-                400 => error_codes([?BAD_REQUEST], <<"Bad Request">>)
+                404 => error_codes([?NOT_FOUND], <<"Not Found">>),
+                500 => error_codes([?INTERNAL_ERROR], <<"Internal Service Error">>)
             }
         }
     };
@@ -576,7 +578,11 @@ authenticator(delete, #{bindings := #{id := AuthenticatorID}}) ->
     delete_authenticator([authentication], ?GLOBAL, AuthenticatorID).
 
 authenticator_status(get, #{bindings := #{id := AuthenticatorID}}) ->
-    lookup_from_all_nodes(?GLOBAL, AuthenticatorID).
+    with_authenticator(
+        AuthenticatorID,
+        [authentication],
+        fun(_) -> lookup_from_all_nodes(?GLOBAL, AuthenticatorID) end
+    ).
 
 listener_authenticators(post, #{bindings := #{listener_id := ListenerID}, body := Config}) ->
     with_listener(
@@ -647,8 +653,12 @@ listener_authenticator_status(
 ) ->
     with_listener(
         ListenerID,
-        fun(_, _, ChainName) ->
-            lookup_from_all_nodes(ChainName, AuthenticatorID)
+        fun(Type, Name, ChainName) ->
+            with_authenticator(
+                AuthenticatorID,
+                [listeners, Type, Name, authentication],
+                fun(_) -> lookup_from_all_nodes(ChainName, AuthenticatorID) end
+            )
         end
     ).
 
@@ -774,6 +784,18 @@ listener_authenticator_user(delete, #{
 %% Internal functions
 %%------------------------------------------------------------------------------
 
+with_authenticator(AuthenticatorID, ConfKeyPath, Fun) ->
+    case find_authenticator_config(AuthenticatorID, ConfKeyPath) of
+        {ok, AuthenticatorConfig} ->
+            Fun(AuthenticatorConfig);
+        {error, Reason} ->
+            serialize_error(Reason)
+    end.
+
+find_authenticator_config(AuthenticatorID, ConfKeyPath) ->
+    AuthenticatorsConfig = get_raw_config_with_defaults(ConfKeyPath),
+    find_config(AuthenticatorID, AuthenticatorsConfig).
+
 with_listener(ListenerID, Fun) ->
     case find_listener(ListenerID) of
         {ok, {BType, BName}} ->
@@ -836,13 +858,13 @@ list_authenticators(ConfKeyPath) ->
     {200, NAuthenticators}.
 
 list_authenticator(_, ConfKeyPath, AuthenticatorID) ->
-    AuthenticatorsConfig = get_raw_config_with_defaults(ConfKeyPath),
-    case find_config(AuthenticatorID, AuthenticatorsConfig) of
-        {ok, AuthenticatorConfig} ->
-            {200, maps:put(id, AuthenticatorID, convert_certs(AuthenticatorConfig))};
-        {error, Reason} ->
-            serialize_error(Reason)
-    end.
+    with_authenticator(
+        AuthenticatorID,
+        ConfKeyPath,
+        fun(AuthenticatorConfig) ->
+            {200, maps:put(id, AuthenticatorID, convert_certs(AuthenticatorConfig))}
+        end
+    ).
 
 resource_provider() ->
     [
@@ -877,7 +899,8 @@ lookup_from_local_node(ChainName, AuthenticatorID) ->
 
 lookup_from_all_nodes(ChainName, AuthenticatorID) ->
     Nodes = mria_mnesia:running_nodes(),
-    case is_ok(emqx_authn_proto_v1:lookup_from_all_nodes(Nodes, ChainName, AuthenticatorID)) of
+    LookupResult = emqx_authn_proto_v1:lookup_from_all_nodes(Nodes, ChainName, AuthenticatorID),
+    case is_ok(LookupResult) of
         {ok, ResList} ->
             {StatusMap, MetricsMap, ResourceMetricsMap, ErrorMap} = make_result_map(ResList),
             AggregateStatus = aggregate_status(maps:values(StatusMap)),
@@ -901,7 +924,7 @@ lookup_from_all_nodes(ChainName, AuthenticatorID) ->
                 node_error => HelpFun(maps:map(Fun, ErrorMap), reason)
             }};
         {error, ErrL} ->
-            {400, #{
+            {500, #{
                 code => <<"INTERNAL_ERROR">>,
                 message => list_to_binary(io_lib:format("~p", [ErrL]))
             }}

apps/emqx_authn/test/emqx_authn_api_SUITE.erl

@@ -39,6 +39,9 @@ all() ->
 groups() ->
     [].
 
+init_per_testcase(t_authenticator_fail, Config) ->
+    meck:expect(emqx_authn_proto_v1, lookup_from_all_nodes, 3, [{error, {exception, badarg}}]),
+    init_per_testcase(default, Config);
 init_per_testcase(_, Config) ->
     {ok, _} = emqx_cluster_rpc:start_link(node(), emqx_cluster_rpc, 1000),
     emqx_authn_test_lib:delete_authenticators(
@@ -54,6 +57,12 @@ init_per_testcase(_, Config) ->
     {atomic, ok} = mria:clear_table(emqx_authn_mnesia),
     Config.
 
+end_per_testcase(t_authenticator_fail, Config) ->
+    meck:unload(emqx_authn_proto_v1),
+    Config;
+end_per_testcase(_, Config) ->
+    Config.
+
 init_per_suite(Config) ->
     emqx_config:erase(?EMQX_AUTHENTICATION_CONFIG_ROOT_NAME_BINARY),
     _ = application:load(emqx_conf),
@@ -90,6 +99,21 @@ t_authenticators(_) ->
 t_authenticator(_) ->
     test_authenticator([]).
 
+t_authenticator_fail(_) ->
+    ValidConfig0 = emqx_authn_test_lib:http_example(),
+    {ok, 200, _} = request(
+        post,
+        uri([?CONF_NS]),
+        ValidConfig0
+    ),
+    ?assertMatch(
+        {ok, 500, _},
+        request(
+            get,
+            uri([?CONF_NS, "password_based:http", "status"])
+        )
+    ).
+
 t_authenticator_users(_) ->
     test_authenticator_users([]).
 
@@ -247,6 +271,15 @@ test_authenticator(PathPrefix) ->
         <<"connected">>,
         LookFun([<<"status">>])
     ),
+
+    ?assertMatch(
+        {ok, 404, _},
+        request(
+            get,
+            uri(PathPrefix ++ [?CONF_NS, "unknown_auth_chain", "status"])
+        )
+    ),
+
     {ok, 404, _} = request(
         get,
         uri(PathPrefix ++ [?CONF_NS, "password_based:redis"])

changes/v5.0.11-en.md

@@ -7,3 +7,4 @@
 
 ## Bug fixes
 
+- Return 404 for status of unknown authenticator in `/authenticator/{id}/status` [#9328](https://github.com/emqx/emqx/pull/9328).

changes/v5.0.11-zh.md

@@ -5,5 +5,6 @@
 - 增强 `保留消息` 的安全性 [#9332](https://github.com/emqx/emqx/pull/9332)。
   现在投递保留消息前，会先过滤掉来源客户端被封禁了的那些消息。
 
-## Bug fixes
+## 修复
 
+- 通过 `/authenticator/{id}/status` 请求未知认证器的状态时，将会返回 404。