From aa019b34ad8e975c3796ebed712777ea109d1680 Mon Sep 17 00:00:00 2001 From: JianBo He Date: Sat, 31 Jul 2021 12:11:34 +0800 Subject: [PATCH] test(exhook): refine tests --- apps/emqx_exhook/include/emqx_exhook.hrl | 2 +- apps/emqx_exhook/priv/emqx_exhook.schema | 38 ------------------ apps/emqx_exhook/rebar.config | 8 ---- apps/emqx_exhook/src/emqx_exhook_cli.erl | 2 +- apps/emqx_exhook/src/emqx_exhook_handler.erl | 7 ++-- apps/emqx_exhook/src/emqx_exhook_server.erl | 1 - apps/emqx_exhook/test/emqx_exhook_SUITE.erl | 39 +++++++++++-------- .../emqx_exhook/test/emqx_exhook_demo_svr.erl | 8 ++-- scripts/find-apps.sh | 2 +- 9 files changed, 33 insertions(+), 74 deletions(-) delete mode 100644 apps/emqx_exhook/priv/emqx_exhook.schema diff --git a/apps/emqx_exhook/include/emqx_exhook.hrl b/apps/emqx_exhook/include/emqx_exhook.hrl index 7301fdcbb..64131735e 100644 --- a/apps/emqx_exhook/include/emqx_exhook.hrl +++ b/apps/emqx_exhook/include/emqx_exhook.hrl @@ -25,7 +25,7 @@ , {'client.connected', {emqx_exhook_handler, on_client_connected, []}} , {'client.disconnected', {emqx_exhook_handler, on_client_disconnected, []}} , {'client.authenticate', {emqx_exhook_handler, on_client_authenticate, []}} - , {'client.check_acl', {emqx_exhook_handler, on_client_check_acl, []}} + , {'client.authorize', {emqx_exhook_handler, on_client_authorize, []}} , {'client.subscribe', {emqx_exhook_handler, on_client_subscribe, []}} , {'client.unsubscribe', {emqx_exhook_handler, on_client_unsubscribe, []}} , {'session.created', {emqx_exhook_handler, on_session_created, []}} diff --git a/apps/emqx_exhook/priv/emqx_exhook.schema b/apps/emqx_exhook/priv/emqx_exhook.schema deleted file mode 100644 index e5481a3dd..000000000 --- a/apps/emqx_exhook/priv/emqx_exhook.schema +++ /dev/null @@ -1,38 +0,0 @@ -%%-*- mode: erlang -*- - -{mapping, "exhook.server.$name.url", "emqx_exhook.servers", [ - {datatype, string} -]}. - -{mapping, "exhook.server.$name.ssl.cacertfile", "emqx_exhook.servers", [ - {datatype, string} -]}. - -{mapping, "exhook.server.$name.ssl.certfile", "emqx_exhook.servers", [ - {datatype, string} -]}. - -{mapping, "exhook.server.$name.ssl.keyfile", "emqx_exhook.servers", [ - {datatype, string} -]}. - -{translation, "emqx_exhook.servers", fun(Conf) -> - Filter = fun(Opts) -> [{K, V} || {K, V} <- Opts, V =/= undefined] end, - ServerOptions = fun(Prefix) -> - case http_uri:parse(cuttlefish:conf_get(Prefix ++ ".url", Conf)) of - {ok, {http, _, Host, Port, _, _}} -> - [{scheme, http}, {host, Host}, {port, Port}]; - {ok, {https, _, Host, Port, _, _}} -> - [{scheme, https}, {host, Host}, {port, Port}, - {ssl_options, - Filter([{ssl, true}, - {certfile, cuttlefish:conf_get(Prefix ++ ".ssl.certfile", Conf, undefined)}, - {keyfile, cuttlefish:conf_get(Prefix ++ ".ssl.keyfile", Conf, undefined)}, - {cacertfile, cuttlefish:conf_get(Prefix ++ ".ssl.cacertfile", Conf, undefined)} - ])}]; - _ -> error(invalid_server_options) - end - end, - [{list_to_atom(Name), ServerOptions("exhook.server." ++ Name)} - || {["exhook", "server", Name, "url"], _} <- cuttlefish_variable:filter_by_prefix("exhook.server", Conf)] -end}. diff --git a/apps/emqx_exhook/rebar.config b/apps/emqx_exhook/rebar.config index 883aad9bd..89dcb20a7 100644 --- a/apps/emqx_exhook/rebar.config +++ b/apps/emqx_exhook/rebar.config @@ -39,11 +39,3 @@ {cover_excl_mods, [emqx_exhook_pb, emqx_exhook_v_1_hook_provider_bhvr, emqx_exhook_v_1_hook_provider_client]}. - -{profiles, - [{test, - [{deps, - [{emqx_ct_helper, {git, "https://github.com/emqx/emqx-ct-helpers", {tag, "v1.3.1"}}} - ]} - ]} -]}. diff --git a/apps/emqx_exhook/src/emqx_exhook_cli.erl b/apps/emqx_exhook/src/emqx_exhook_cli.erl index a8dc43b16..3f0dd0b6c 100644 --- a/apps/emqx_exhook/src/emqx_exhook_cli.erl +++ b/apps/emqx_exhook/src/emqx_exhook_cli.erl @@ -30,7 +30,7 @@ cli(["server", "list"]) -> cli(["server", "enable", Name0]) -> if_enabled(fun() -> Name = list_to_atom(Name0), - case proplists:get_value(Name, application:get_env(?APP, servers, [])) of + case maps:get(Name, emqx_config:get([exhook, server]), undefined) of undefined -> emqx_ctl:print("not_found~n"); Opts -> diff --git a/apps/emqx_exhook/src/emqx_exhook_handler.erl b/apps/emqx_exhook/src/emqx_exhook_handler.erl index d2fb84bab..1e81646e0 100644 --- a/apps/emqx_exhook/src/emqx_exhook_handler.erl +++ b/apps/emqx_exhook/src/emqx_exhook_handler.erl @@ -87,7 +87,6 @@ on_client_disconnected(ClientInfo, Reason, _ConnInfo) -> }, cast('client.disconnected', Req). -%% FIXME: `AuthResult` on_client_authenticate(ClientInfo, AuthResult) -> %% XXX: Bool is missing more information about the atom of the result %% So, the `Req` has missed detailed info too. @@ -95,7 +94,7 @@ on_client_authenticate(ClientInfo, AuthResult) -> %% The return value of `call_fold` just a bool, that has missed %% detailed info too. %% - Bool = maps:get(auth_result, AuthResult, undefined) == success, + Bool = AuthResult == ok, Req = #{clientinfo => clientinfo(ClientInfo), result => Bool }, @@ -103,8 +102,8 @@ on_client_authenticate(ClientInfo, AuthResult) -> case call_fold('client.authenticate', Req, fun merge_responsed_bool/2) of {StopOrOk, #{result := Result0}} when is_boolean(Result0) -> - Result = case Result0 of true -> success; _ -> not_authorized end, - {StopOrOk, AuthResult#{auth_result => Result, anonymous => false}}; + Result = case Result0 of true -> ok; _ -> {error, not_authorized} end, + {StopOrOk, Result}; _ -> {ok, AuthResult} end. diff --git a/apps/emqx_exhook/src/emqx_exhook_server.erl b/apps/emqx_exhook/src/emqx_exhook_server.erl index 5bda76d13..674bfea5a 100644 --- a/apps/emqx_exhook/src/emqx_exhook_server.erl +++ b/apps/emqx_exhook/src/emqx_exhook_server.erl @@ -122,7 +122,6 @@ to_list(Name) when is_list(Name) -> %% @private channel_opts(Opts = #{url := URL}) -> - io:format("~p~n", [Opts]), case uri_string:parse(URL) of #{scheme := <<"http">>, host := Host, port := Port} -> {format_http_uri("http", Host, Port), #{}}; diff --git a/apps/emqx_exhook/test/emqx_exhook_SUITE.erl b/apps/emqx_exhook/test/emqx_exhook_SUITE.erl index 5d5a396a5..0291a8f91 100644 --- a/apps/emqx_exhook/test/emqx_exhook_SUITE.erl +++ b/apps/emqx_exhook/test/emqx_exhook_SUITE.erl @@ -22,6 +22,10 @@ -include_lib("eunit/include/eunit.hrl"). -include_lib("common_test/include/ct.hrl"). +-define(CONF_DEFAULT, <<" +exhook: { server.default: { url: \"http://127.0.0.1:9000\" } } +">>). + %%-------------------------------------------------------------------- %% Setups %%-------------------------------------------------------------------- @@ -30,35 +34,24 @@ all() -> emqx_ct:all(?MODULE). init_per_suite(Cfg) -> _ = emqx_exhook_demo_svr:start(), - emqx_ct_helpers:start_apps([emqx_exhook], fun set_special_cfgs/1), + ok = emqx_config:init_load(emqx_exhook_schema, ?CONF_DEFAULT), + emqx_ct_helpers:start_apps([emqx_exhook]), Cfg. end_per_suite(_Cfg) -> emqx_ct_helpers:stop_apps([emqx_exhook]), emqx_exhook_demo_svr:stop(). -set_special_cfgs(emqx) -> - application:set_env(emqx, allow_anonymous, false), - application:set_env(emqx, enable_acl_cache, false), - application:set_env(emqx, plugins_loaded_file, undefined), - application:set_env(emqx, modules_loaded_file, undefined); -set_special_cfgs(emqx_exhook) -> - ok. - %%-------------------------------------------------------------------- %% Test cases %%-------------------------------------------------------------------- t_noserver_nohook(_) -> emqx_exhook:disable(default), - ?assertEqual([], ets:tab2list(emqx_hooks)), - - Opts = proplists:get_value( - default, - application:get_env(emqx_exhook, servers, []) - ), + ?assertEqual([], loaded_exhook_hookpoints()), + Opts = emqx_config:get([exhook, server, default]), ok = emqx_exhook:enable(default, Opts), - ?assertNotEqual([], ets:tab2list(emqx_hooks)). + ?assertNotEqual([], loaded_exhook_hookpoints()). t_cli_list(_) -> meck_print(), @@ -94,3 +87,17 @@ meck_print() -> unmeck_print() -> meck:unload(emqx_ctl). + +loaded_exhook_hookpoints() -> + lists:filtermap(fun(E) -> + Name = element(2, E), + Callbacks = element(3, E), + case lists:any(fun is_exhook_callback/1, Callbacks) of + true -> {true, Name}; + _ -> false + end + end, ets:tab2list(emqx_hooks)). + +is_exhook_callback(Cb) -> + Action = element(2, Cb), + emqx_exhook_handler == element(1, Action). diff --git a/apps/emqx_exhook/test/emqx_exhook_demo_svr.erl b/apps/emqx_exhook/test/emqx_exhook_demo_svr.erl index c2db04dd4..656788b5e 100644 --- a/apps/emqx_exhook/test/emqx_exhook_demo_svr.erl +++ b/apps/emqx_exhook/test/emqx_exhook_demo_svr.erl @@ -33,7 +33,7 @@ , on_client_connected/2 , on_client_disconnected/2 , on_client_authenticate/2 - , on_client_check_acl/2 + , on_client_authorize/2 , on_client_subscribe/2 , on_client_unsubscribe/2 , on_session_created/2 @@ -122,7 +122,7 @@ on_provider_loaded(Req, Md) -> #{name => <<"client.connected">>}, #{name => <<"client.disconnected">>}, #{name => <<"client.authenticate">>}, - #{name => <<"client.check_acl">>}, + #{name => <<"client.authorize">>}, #{name => <<"client.subscribe">>}, #{name => <<"client.unsubscribe">>}, #{name => <<"session.created">>}, @@ -197,10 +197,10 @@ on_client_authenticate(#{clientinfo := #{username := Username}} = Req, Md) -> {ok, #{type => 'IGNORE'}, Md} end. --spec on_client_check_acl(emqx_exhook_pb:client_check_acl_request(), grpc:metadata()) +-spec on_client_authorize(emqx_exhook_pb:client_authorize_request(), grpc:metadata()) -> {ok, emqx_exhook_pb:valued_response(), grpc:metadata()} | {error, grpc_cowboy_h:error_response()}. -on_client_check_acl(#{clientinfo := #{username := Username}} = Req, Md) -> +on_client_authorize(#{clientinfo := #{username := Username}} = Req, Md) -> ?MODULE:in({?FUNCTION_NAME, Req}), %io:format("fun: ~p, req: ~0p~n", [?FUNCTION_NAME, Req]), %% some cases for testing diff --git a/scripts/find-apps.sh b/scripts/find-apps.sh index e437c49c5..177a4f57c 100755 --- a/scripts/find-apps.sh +++ b/scripts/find-apps.sh @@ -7,7 +7,7 @@ cd -P -- "$(dirname -- "$0")/.." find_app() { local appdir="$1" - find "${appdir}" -mindepth 1 -maxdepth 1 -type d | grep -vE "emqx_exhook|emqx_exproto|emqx_lwm2m|emqx_sn|emqx_coap|emqx_stomp|emqx_dashboard" + find "${appdir}" -mindepth 1 -maxdepth 1 -type d | grep -vE "emqx_dashboard" } find_app 'apps'