Merge pull request #8780 from HJianBo/authz-ignore-nomatch-rules

fix: exhook client.authorize never be execauted

CHANGES-5.0.md

@@ -1,5 +1,9 @@
 # 5.0.8
 
+## Bug fixes
+
+* Fix exhook `client.authorize` never being execauted. [#8780](https://github.com/emqx/emqx/pull/8780)
+
 ## Enhancements
 
 * change the `/gateway` API path to plural form. [#8823](https://github.com/emqx/emqx/pull/8823)

apps/emqx/src/emqx_access_control.erl

@@ -17,6 +17,7 @@
 -module(emqx_access_control).
 
 -include("emqx.hrl").
+-include("logger.hrl").
 
 -export([
     authenticate/1,
@@ -70,9 +71,26 @@ check_authorization_cache(ClientInfo, PubSub, Topic) ->
 
 do_authorize(ClientInfo, PubSub, Topic) ->
     NoMatch = emqx:get_config([authorization, no_match], allow),
-    case run_hooks('client.authorize', [ClientInfo, PubSub, Topic], NoMatch) of
+    Default = #{result => NoMatch, from => default},
-        allow -> allow;
+    case run_hooks('client.authorize', [ClientInfo, PubSub, Topic], Default) of
-        _Other -> deny
+        AuthzResult = #{result := Result} when Result == allow; Result == deny ->
+            From = maps:get(from, AuthzResult, unknown),
+            emqx:run_hook(
+                'client.check_authz_complete',
+                [ClientInfo, PubSub, Topic, Result, From]
+            ),
+            Result;
+        Other ->
+            ?SLOG(error, #{
+                msg => "unknown_authorization_return_format",
+                expected_example => "#{result => allow, from => default}",
+                got => Other
+            }),
+            emqx:run_hook(
+                'client.check_authz_complete',
+                [ClientInfo, PubSub, Topic, deny, unknown_return_format]
+            ),
+            deny
     end.
 
 -compile({inline, [run_hooks/3]}).

apps/emqx/test/emqx_proper_types.erl

@@ -69,6 +69,7 @@ conninfo() ->
         {conn_props, properties()},
         {connected, boolean()},
         {connected_at, timestamp()},
+        {disconnected_at, timestamp()},
         {keepalive, range(0, 16#ffff)},
         {receive_maximum, non_neg_integer()},
         {expiry_interval, non_neg_integer()}

apps/emqx_authz/src/emqx_authz.app.src

@@ -1,7 +1,7 @@
 %% -*- mode: erlang -*-
 {application, emqx_authz, [
     {description, "An OTP application"},
-    {vsn, "0.1.4"},
+    {vsn, "0.1.5"},
     {registered, []},
     {mod, {emqx_authz_app, []}},
     {applications, [

apps/emqx_authz/src/emqx_authz.erl

@@ -49,7 +49,8 @@
 
 -type default_result() :: allow | deny.
 
--type authz_result() :: {stop, allow} | {ok, deny}.
+-type authz_result_value() :: #{result := allow | deny, from => _}.
+-type authz_result() :: {stop, authz_result_value()} | {ok, authz_result_value()} | ignore.
 
 -type sources() :: [source()].
 
@@ -319,7 +320,7 @@ authorize(
                 is_superuser => true
             }),
             emqx_metrics:inc(?METRIC_SUPERUSER),
-            {stop, allow};
+            {stop, #{result => allow, from => superuser}};
         false ->
             authorize_non_superuser(Client, PubSub, Topic, DefaultResult, Sources)
     end.
@@ -331,15 +332,11 @@ authorize_non_superuser(
     } = Client,
     PubSub,
     Topic,
-    DefaultResult,
+    _DefaultResult,
     Sources
 ) ->
     case do_authorize(Client, PubSub, Topic, sources_with_defaults(Sources)) of
         {{matched, allow}, AuthzSource} ->
-            emqx:run_hook(
-                'client.check_authz_complete',
-                [Client, PubSub, Topic, allow, AuthzSource]
-            ),
             log_allowed(#{
                 username => Username,
                 ipaddr => IpAddress,
@@ -348,12 +345,8 @@ authorize_non_superuser(
             }),
             emqx_metrics_worker:inc(authz_metrics, AuthzSource, allow),
             emqx_metrics:inc(?METRIC_ALLOW),
-            {stop, allow};
+            {stop, #{result => allow, from => AuthzSource}};
         {{matched, deny}, AuthzSource} ->
-            emqx:run_hook(
-                'client.check_authz_complete',
-                [Client, PubSub, Topic, deny, AuthzSource]
-            ),
             ?SLOG(warning, #{
                 msg => "authorization_permission_denied",
                 username => Username,
@@ -363,12 +356,8 @@ authorize_non_superuser(
             }),
             emqx_metrics_worker:inc(authz_metrics, AuthzSource, deny),
             emqx_metrics:inc(?METRIC_DENY),
-            {stop, deny};
+            {stop, #{result => deny, from => AuthzSource}};
         nomatch ->
-            emqx:run_hook(
-                'client.check_authz_complete',
-                [Client, PubSub, Topic, DefaultResult, default]
-            ),
             ?SLOG(info, #{
                 msg => "authorization_failed_nomatch",
                 username => Username,
@@ -377,7 +366,7 @@ authorize_non_superuser(
                 reason => "no-match rule"
             }),
             emqx_metrics:inc(?METRIC_NOMATCH),
-            {stop, DefaultResult}
+            ignore
     end.
 
 log_allowed(Meta) ->

apps/emqx_exhook/src/emqx_exhook_handler.erl

@@ -133,7 +133,7 @@ on_client_authenticate(ClientInfo, AuthResult) ->
     end.
 
 on_client_authorize(ClientInfo, PubSub, Topic, Result) ->
-    Bool = Result == allow,
+    Bool = maps:get(result, Result, deny) == allow,
     Type =
         case PubSub of
             publish -> 'PUBLISH';
@@ -158,7 +158,7 @@ on_client_authorize(ClientInfo, PubSub, Topic, Result) ->
                     true -> allow;
                     _ -> deny
                 end,
-            {StopOrOk, NResult};
+            {StopOrOk, #{result => NResult, from => exhook}};
         _ ->
             {ok, Result}
     end.

apps/emqx_exhook/test/emqx_exhook_SUITE.erl

@@ -23,6 +23,7 @@
 
 -include_lib("eunit/include/eunit.hrl").
 -include_lib("common_test/include/ct.hrl").
+-include_lib("emqx/include/emqx_hooks.hrl").
 
 -define(DEFAULT_CLUSTER_NAME_ATOM, emqxcl).
 
@@ -105,7 +106,10 @@ load_cfg(Cfg) ->
 %%--------------------------------------------------------------------
 
 t_access_failed_if_no_server_running(Config) ->
-    emqx_exhook_mgr:disable(<<"default">>),
+    meck:expect(emqx_metrics_worker, inc, fun(_, _, _) -> ok end),
+    meck:expect(emqx_metrics, inc, fun(_) -> ok end),
+    emqx_hooks:add('client.authorize', {emqx_authz, authorize, [[]]}, ?HP_AUTHZ),
+
     ClientInfo = #{
         clientid => <<"user-id-1">>,
         username => <<"usera">>,
@@ -114,14 +118,35 @@ t_access_failed_if_no_server_running(Config) ->
         protocol => mqtt,
         mountpoint => undefined
     },
+    ?assertMatch(
+        allow,
+        emqx_access_control:authorize(
+            ClientInfo#{username => <<"gooduser">>},
+            publish,
+            <<"acl/1">>
+        )
+    ),
+
+    ?assertMatch(
+        deny,
+        emqx_access_control:authorize(
+            ClientInfo#{username => <<"baduser">>},
+            publish,
+            <<"acl/2">>
+        )
+    ),
+
+    emqx_exhook_mgr:disable(<<"default">>),
     ?assertMatch(
         {stop, {error, not_authorized}},
         emqx_exhook_handler:on_client_authenticate(ClientInfo, #{auth_result => success})
     ),
 
     ?assertMatch(
-        {stop, deny},
+        {stop, #{result := deny, from := exhook}},
-        emqx_exhook_handler:on_client_authorize(ClientInfo, publish, <<"t/1">>, allow)
+        emqx_exhook_handler:on_client_authorize(ClientInfo, publish, <<"t/1">>, #{
+            result => allow, from => exhook
+        })
     ),
 
     Message = emqx_message:make(<<"t/1">>, <<"abc">>),
@@ -130,6 +155,7 @@ t_access_failed_if_no_server_running(Config) ->
         emqx_exhook_handler:on_message_publish(Message)
     ),
     emqx_exhook_mgr:enable(<<"default">>),
+    emqx_hooks:del('client.authorize', {emqx_authz, authorize}),
     assert_get_basic_usage_info(Config).
 
 t_lookup(_) ->

apps/emqx_exhook/test/props/prop_exhook_hooks.erl

@@ -133,9 +133,16 @@ prop_client_authenticate() ->
     ).
 
 prop_client_authorize() ->
+    MkResult = fun(Result) -> #{result => Result, from => exhook} end,
     ?ALL(
         {ClientInfo0, PubSub, Topic, Result, Meta},
-        {clientinfo(), oneof([publish, subscribe]), topic(), oneof([allow, deny]), request_meta()},
+        {
+            clientinfo(),
+            oneof([publish, subscribe]),
+            topic(),
+            oneof([MkResult(allow), MkResult(deny)]),
+            request_meta()
+        },
         begin
             ClientInfo = inject_magic_into(username, ClientInfo0),
             OutResult = emqx_hooks:run_fold(
@@ -145,9 +152,9 @@ prop_client_authorize() ->
             ),
             ExpectedOutResult =
                 case maps:get(username, ClientInfo) of
-                    <<"baduser">> -> deny;
+                    <<"baduser">> -> MkResult(deny);
-                    <<"gooduser">> -> allow;
+                    <<"gooduser">> -> MkResult(allow);
-                    <<"normaluser">> -> allow;
+                    <<"normaluser">> -> MkResult(allow);
                     _ -> Result
                 end,
             ?assertEqual(ExpectedOutResult, OutResult),
@@ -544,7 +551,7 @@ subopts(SubOpts) ->
 authresult_to_bool(AuthResult) ->
     AuthResult == ok.
 
-aclresult_to_bool(Result) ->
+aclresult_to_bool(#{result := Result}) ->
     Result == allow.
 
 pubsub_to_enum(publish) -> 'PUBLISH';