fix(auth): fix emqx_authenticator cooperation with other 'client.authenticate' hooks

apps/emqx/src/emqx_access_control.erl

@@ -46,16 +46,32 @@ authenticate(Credential) ->
     NotSuperUser = #{is_superuser => false},
     case emqx_authentication:pre_hook_authenticate(Credential) of
         ok ->
+            inc_authn_metrics(anonymous),
             {ok, NotSuperUser};
         continue ->
-            case run_hooks('client.authenticate', [Credential], {ok, #{is_superuser => false}}) of
+            case run_hooks('client.authenticate', [Credential], ignore) of
-                ok ->
+                ignore ->
+                    inc_authn_metrics(anonymous),
                     {ok, NotSuperUser};
+                ok ->
+                    inc_authn_metrics(ok),
+                    {ok, NotSuperUser};
+                {ok, _AuthResult} = OkResult ->
+                    inc_authn_metrics(ok),
+                    OkResult;
+                {ok, _AuthResult, _AuthData} = OkResult ->
+                    inc_authn_metrics(ok),
+                    OkResult;
+                {error, _Reason} = Error ->
+                    inc_authn_metrics(error),
+                    Error;
+                %% {continue, AuthCache} | {continue, AuthData, AuthCache}
                 Other ->
                     Other
             end;
-        Other ->
+        {error, _Reason} = Error ->
-            Other
+            inc_authn_metrics(error),
+            Error
     end.
 
 %% @doc Check Authorization
@@ -134,3 +150,11 @@ inc_authz_metrics(deny) ->
     emqx_metrics:inc('authorization.deny');
 inc_authz_metrics(cache_hit) ->
     emqx_metrics:inc('authorization.cache_hit').
+
+inc_authn_metrics(error) ->
+    emqx_metrics:inc('authentication.failure');
+inc_authn_metrics(ok) ->
+    emqx_metrics:inc('authentication.success');
+inc_authn_metrics(anonymous) ->
+    emqx_metrics:inc('authentication.success.anonymous'),
+    emqx_metrics:inc('authentication.success').

apps/emqx/src/emqx_authentication.erl

@@ -228,7 +228,6 @@ when
 -spec pre_hook_authenticate(emqx_types:clientinfo()) ->
     ok | continue | {error, not_authorized}.
 pre_hook_authenticate(#{enable_authn := false}) ->
-    inc_authenticate_metric('authentication.success.anonymous'),
     ?TRACE_RESULT("authentication_result", ok, enable_authn_false);
 pre_hook_authenticate(#{enable_authn := quick_deny_anonymous} = Credential) ->
     case maps:get(username, Credential, undefined) of
@@ -242,29 +241,18 @@ pre_hook_authenticate(#{enable_authn := quick_deny_anonymous} = Credential) ->
 pre_hook_authenticate(_) ->
     continue.
 
-authenticate(#{listener := Listener, protocol := Protocol} = Credential, _AuthResult) ->
+authenticate(#{listener := Listener, protocol := Protocol} = Credential, AuthResult) ->
     case get_authenticators(Listener, global_chain(Protocol)) of
         {ok, ChainName, Authenticators} ->
             case get_enabled(Authenticators) of
                 [] ->
-                    inc_authenticate_metric('authentication.success.anonymous'),
+                    ?TRACE_RESULT("authentication_result", AuthResult, empty_chain);
-                    ?TRACE_RESULT("authentication_result", ignore, empty_chain);
                 NAuthenticators ->
                     Result = do_authenticate(ChainName, NAuthenticators, Credential),
-
-                    case Result of
-                        {stop, {ok, _}} ->
-                            inc_authenticate_metric('authentication.success');
-                        {stop, {error, _}} ->
-                            inc_authenticate_metric('authentication.failure');
-                        _ ->
-                            ok
-                    end,
                     ?TRACE_RESULT("authentication_result", Result, chain_result)
             end;
         none ->
-            inc_authenticate_metric('authentication.success.anonymous'),
+            ?TRACE_RESULT("authentication_result", AuthResult, no_chain)
-            ?TRACE_RESULT("authentication_result", ignore, no_chain)
     end.
 
 get_authenticators(Listener, Global) ->
@@ -649,7 +637,7 @@ handle_create_authenticator(Chain, Config, Providers) ->
     end.
 
 do_authenticate(_ChainName, [], _) ->
-    {stop, {error, not_authorized}};
+    {ok, {error, not_authorized}};
 do_authenticate(
     ChainName, [#authenticator{id = ID} = Authenticator | More], Credential
 ) ->
@@ -673,7 +661,7 @@ do_authenticate(
                 _ ->
                     ok
             end,
-            {stop, Result}
+            {ok, Result}
     catch
         Class:Reason:Stacktrace ->
             ?TRACE_AUTHN(warning, "authenticator_error", #{
@@ -947,9 +935,3 @@ to_list(M) when is_map(M) -> [M];
 to_list(L) when is_list(L) -> L.
 
 call(Call) -> gen_server:call(?MODULE, Call, infinity).
-
-inc_authenticate_metric('authentication.success.anonymous' = Metric) ->
-    emqx_metrics:inc(Metric),
-    emqx_metrics:inc('authentication.success');
-inc_authenticate_metric(Metric) ->
-    emqx_metrics:inc(Metric).

apps/emqx/test/emqx_authentication_SUITE.erl

@@ -22,6 +22,8 @@
 -compile(export_all).
 -compile(nowarn_export_all).
 
+-include_lib("emqx/include/emqx_hooks.hrl").
+
 -include_lib("common_test/include/ct.hrl").
 -include_lib("eunit/include/eunit.hrl").
 -include_lib("typerefl/include/types.hrl").
@@ -35,6 +37,20 @@
     end)()
 ).
 -define(CONF_ROOT, ?EMQX_AUTHENTICATION_CONFIG_ROOT_NAME_ATOM).
+-define(NOT_SUPERUSER, #{is_superuser => false}).
+
+-define(assertAuthSuccessForUser(User),
+    ?assertMatch(
+        {ok, _},
+        emqx_access_control:authenticate(ClientInfo#{username => atom_to_binary(User)})
+    )
+).
+-define(assertAuthFailureForUser(User),
+    ?assertMatch(
+        {error, _},
+        emqx_access_control:authenticate(ClientInfo#{username => atom_to_binary(User)})
+    )
+).
 
 %%------------------------------------------------------------------------------
 %% Hocon Schema
@@ -88,9 +104,22 @@ update(_Config, _State) ->
 
 authenticate(#{username := <<"good">>}, _State) ->
     {ok, #{is_superuser => true}};
+authenticate(#{username := <<"ignore">>}, _State) ->
+    ignore;
 authenticate(#{username := _}, _State) ->
     {error, bad_username_or_password}.
 
+hook_authenticate(#{username := <<"hook_user_good">>}, _AuthResult) ->
+    {ok, {ok, ?NOT_SUPERUSER}};
+hook_authenticate(#{username := <<"hook_user_bad">>}, _AuthResult) ->
+    {ok, {error, invalid_username}};
+hook_authenticate(#{username := <<"hook_user_finally_good">>}, _AuthResult) ->
+    {stop, {ok, ?NOT_SUPERUSER}};
+hook_authenticate(#{username := <<"hook_user_finally_bad">>}, _AuthResult) ->
+    {stop, {error, invalid_username}};
+hook_authenticate(_ClientId, AuthResult) ->
+    {ok, AuthResult}.
+
 destroy(_State) ->
     ok.
 
@@ -113,6 +142,10 @@ end_per_testcase(Case, Config) ->
     _ = ?MODULE:Case({'end', Config}),
     ok.
 
+%%=================================================================================
+%% Testcases
+%%=================================================================================
+
 t_chain({'init', Config}) ->
     Config;
 t_chain(Config) when is_list(Config) ->
@@ -500,6 +533,92 @@ t_convert_certs(Config) when is_list(Config) ->
     clear_certs(CertsDir, #{<<"ssl">> => NCerts3}),
     ?assertEqual(false, filelib:is_regular(maps:get(<<"keyfile">>, NCerts3))).
 
+t_combine_authn_and_callback({init, Config}) ->
+    [
+        {listener_id, 'tcp:default'},
+        {authn_type, {password_based, built_in_database}}
+        | Config
+    ];
+t_combine_authn_and_callback(Config) when is_list(Config) ->
+    ListenerID = ?config(listener_id),
+    ClientInfo = #{
+        zone => default,
+        listener => ListenerID,
+        protocol => mqtt,
+        password => <<"any">>
+    },
+
+    %% no emqx_authentication authenticators, anonymous is allowed
+    ?assertAuthSuccessForUser(bad),
+
+    AuthNType = ?config(authn_type),
+    register_provider(AuthNType, ?MODULE),
+
+    AuthenticatorConfig = #{
+        mechanism => password_based,
+        backend => built_in_database,
+        enable => true
+    },
+    {ok, _} = ?AUTHN:create_authenticator(ListenerID, AuthenticatorConfig),
+
+    %% emqx_authentication alone
+    ?assertAuthSuccessForUser(good),
+    ?assertAuthFailureForUser(ignore),
+    ?assertAuthFailureForUser(bad),
+
+    %% add hook with higher priority
+    ok = hook(?HP_AUTHN + 1),
+
+    %% for hook unrelataed users everything is the same
+    ?assertAuthSuccessForUser(good),
+    ?assertAuthFailureForUser(ignore),
+    ?assertAuthFailureForUser(bad),
+
+    %% higher-priority hook can permit access with {ok,...},
+    %% then emqx_authentication overrides the result
+    ?assertAuthFailureForUser(hook_user_good),
+    ?assertAuthFailureForUser(hook_user_bad),
+
+    %% higher-priority hook can permit and return {stop,...},
+    %% then emqx_authentication cannot override the result
+    ?assertAuthSuccessForUser(hook_user_finally_good),
+    ?assertAuthFailureForUser(hook_user_finally_bad),
+
+    ok = unhook(),
+
+    %% add hook with lower priority
+    ok = hook(?HP_AUTHN - 1),
+
+    %% for hook unrelataed users
+    ?assertAuthSuccessForUser(good),
+    ?assertAuthFailureForUser(bad),
+    ?assertAuthFailureForUser(ignore),
+
+    %% lower-priority hook can overrride auth result,
+    %% because emqx_authentication permits/denies with {ok, ...}
+    ?assertAuthSuccessForUser(hook_user_good),
+    ?assertAuthFailureForUser(hook_user_bad),
+    ?assertAuthSuccessForUser(hook_user_finally_good),
+    ?assertAuthFailureForUser(hook_user_finally_bad),
+
+    ok = unhook();
+t_combine_authn_and_callback({'end', Config}) ->
+    ?AUTHN:delete_chain(?config(listener_id)),
+    ?AUTHN:deregister_provider(?config(authn_type)),
+    ok.
+
+%%=================================================================================
+%% Helpers fns
+%%=================================================================================
+
+hook(Priority) ->
+    ok = emqx_hooks:put(
+        'client.authenticate', {?MODULE, hook_authenticate, []}, Priority
+    ).
+
+unhook() ->
+    ok = emqx_hooks:del('client.authenticate', {?MODULE, hook_authenticate}).
+
 update_config(Path, ConfigRequest) ->
     emqx:update_config(Path, ConfigRequest, #{rawconf_with_defaults => true}).
 

changes/v5.0.12-en.md

@@ -33,3 +33,6 @@
   - Fixed the `Discovery error: no such service` error occurred during helm chart deployment, resulting in an abnormal discovery of cluster nodes.
 
   - Fixed that caused EMQX Helm Chart to fail when modifying some of EMQX's configuration items via environment variables
+
+- Fix shadowing `'client.authenticate'` callbacks by `emqx_authenticator`. Now `emqx_authenticator`
+  passes execution to the further callbacks if none of the authenticators matches [#9496](https://github.com/emqx/emqx/pull/9496).

changes/v5.0.12-zh.md

@@ -33,3 +33,5 @@
   - 修复了 EMQX Helm Chart 部署时出现 `Discovery error: no such service` 错误，导致集群节点发现异常。
 
   - 修复了 EMQX Helm Chart 通过环境变量修改部分 EMQX 的配置项时的错误
+
+- 通过 `emqx_authenticator` 修复隐藏 `'client.authenticate'` 回调。 现在 `emqx_authenticator` 如果没有任何验证器匹配，则将执行传递给进一步的回调 [#9496](https://github.com/emqx/emqx/pull/9496)。