diff --git a/CHANGES-5.0.md b/CHANGES-5.0.md index 742883e27..1c6c5dc64 100644 --- a/CHANGES-5.0.md +++ b/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) diff --git a/apps/emqx/src/emqx_access_control.erl b/apps/emqx/src/emqx_access_control.erl index 1b864cda9..1345c78e0 100644 --- a/apps/emqx/src/emqx_access_control.erl +++ b/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 - allow -> allow; - _Other -> deny + Default = #{result => NoMatch, from => default}, + case run_hooks('client.authorize', [ClientInfo, PubSub, Topic], Default) of + 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]}). diff --git a/apps/emqx/test/emqx_proper_types.erl b/apps/emqx/test/emqx_proper_types.erl index b56140288..78fab0b38 100644 --- a/apps/emqx/test/emqx_proper_types.erl +++ b/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()} diff --git a/apps/emqx_authz/src/emqx_authz.app.src b/apps/emqx_authz/src/emqx_authz.app.src index e40b5e64c..4a2186066 100644 --- a/apps/emqx_authz/src/emqx_authz.app.src +++ b/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, [ diff --git a/apps/emqx_authz/src/emqx_authz.erl b/apps/emqx_authz/src/emqx_authz.erl index 8eb3c6eae..3b175fc31 100644 --- a/apps/emqx_authz/src/emqx_authz.erl +++ b/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) -> diff --git a/apps/emqx_exhook/src/emqx_exhook_handler.erl b/apps/emqx_exhook/src/emqx_exhook_handler.erl index eace7a52f..ddf25cfdf 100644 --- a/apps/emqx_exhook/src/emqx_exhook_handler.erl +++ b/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. diff --git a/apps/emqx_exhook/test/emqx_exhook_SUITE.erl b/apps/emqx_exhook/test/emqx_exhook_SUITE.erl index 74ec6c1ae..62606cf18 100644 --- a/apps/emqx_exhook/test/emqx_exhook_SUITE.erl +++ b/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}, - emqx_exhook_handler:on_client_authorize(ClientInfo, publish, <<"t/1">>, allow) + {stop, #{result := deny, from := exhook}}, + 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(_) -> diff --git a/apps/emqx_exhook/test/props/prop_exhook_hooks.erl b/apps/emqx_exhook/test/props/prop_exhook_hooks.erl index 5c169f72f..c42791f7a 100644 --- a/apps/emqx_exhook/test/props/prop_exhook_hooks.erl +++ b/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; - <<"gooduser">> -> allow; - <<"normaluser">> -> allow; + <<"baduser">> -> MkResult(deny); + <<"gooduser">> -> MkResult(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';