From 15035f7eb074185ac88cbc7d2d9da553c0e60932 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Sun, 8 Jan 2023 21:57:53 +0100 Subject: [PATCH 01/11] refactor: remove lazy type for authentication The idea of using lazy type for authentication was to make the config check easy to extend at runtime. However the reality is: it's overly complicated. It's more likely that people will just continue to implementing the auth hook callbacks instead of injecting config schema at runtime. --- apps/emqx/src/emqx_config.erl | 14 ++-------- apps/emqx/src/emqx_schema.erl | 27 +++++++------------ apps/emqx_conf/src/emqx_conf.erl | 2 -- .../src/emqx_dashboard_swagger.erl | 2 -- 4 files changed, 12 insertions(+), 33 deletions(-) diff --git a/apps/emqx/src/emqx_config.erl b/apps/emqx/src/emqx_config.erl index 6d706316c..c462b6725 100644 --- a/apps/emqx/src/emqx_config.erl +++ b/apps/emqx/src/emqx_config.erl @@ -366,13 +366,6 @@ schema_default(Schema) -> case hocon_schema:field_schema(Schema, type) of ?ARRAY(_) -> []; - ?LAZY(?ARRAY(_)) -> - []; - ?LAZY(?UNION(Members)) -> - case [A || ?ARRAY(A) <- hoconsc:union_members(Members)] of - [_ | _] -> []; - _ -> #{} - end; _ -> #{} end. @@ -407,8 +400,7 @@ merge_envs(SchemaMod, RawConf) -> Opts = #{ required => false, format => map, - apply_override_envs => true, - check_lazy => true + apply_override_envs => true }, hocon_tconf:merge_env_overrides(SchemaMod, RawConf, all, Opts). @@ -451,9 +443,7 @@ compact_errors(Schema, Errors) -> do_check_config(SchemaMod, RawConf, Opts0) -> Opts1 = #{ return_plain => true, - format => map, - %% Don't check lazy types, such as authenticate - check_lazy => false + format => map }, Opts = maps:merge(Opts0, Opts1), {AppEnvs, CheckedConf} = diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl index 48bd206c9..77b51f074 100644 --- a/apps/emqx/src/emqx_schema.erl +++ b/apps/emqx/src/emqx_schema.erl @@ -2352,25 +2352,18 @@ authentication(Which) -> global -> ?DESC(global_authentication); listener -> ?DESC(listener_authentication) end, - %% The runtime module injection - %% from EMQX_AUTHENTICATION_SCHEMA_MODULE_PT_KEY - %% is for now only affecting document generation. - %% maybe in the future, we can find a more straightforward way to support - %% * document generation (at compile time) - %% * type checks before boot (in bin/emqx config generation) - %% * type checks at runtime (when changing configs via management API) - Type0 = + %% poor man's dependency injection + %% this is due to the fact that authn is implemented outside of 'emqx' app. + %% so it can not be a part of emqx_schema since 'emqx' app is supposed to + %% work standalone. + Type = case persistent_term:get(?EMQX_AUTHENTICATION_SCHEMA_MODULE_PT_KEY, undefined) of - undefined -> hoconsc:array(typerefl:map()); - Module -> Module:root_type() + undefined -> + hoconsc:array(typerefl:map()); + Module -> + Module:root_type() end, - %% It is a lazy type because when handling runtime update requests - %% the config is not checked by emqx_schema, but by the injected schema - Type = hoconsc:lazy(Type0), - #{ - type => Type, - desc => Desc - }. + hoconsc:mk(Type, #{desc => Desc}). -spec qos() -> typerefl:type(). qos() -> diff --git a/apps/emqx_conf/src/emqx_conf.erl b/apps/emqx_conf/src/emqx_conf.erl index 8b471a137..00648db31 100644 --- a/apps/emqx_conf/src/emqx_conf.erl +++ b/apps/emqx_conf/src/emqx_conf.erl @@ -296,8 +296,6 @@ hocon_schema_to_spec(Type, LocalModule) when ?IS_TYPEREFL(Type) -> hocon_schema_to_spec(?ARRAY(Item), LocalModule) -> {Schema, Refs} = hocon_schema_to_spec(Item, LocalModule), {#{type => array, items => Schema}, Refs}; -hocon_schema_to_spec(?LAZY(Item), LocalModule) -> - hocon_schema_to_spec(Item, LocalModule); hocon_schema_to_spec(?ENUM(Items), _LocalModule) -> {#{type => enum, symbols => Items}, []}; hocon_schema_to_spec(?MAP(Name, Type), LocalModule) -> diff --git a/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl b/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl index 0afbf5f1e..1d0fa7352 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl @@ -609,8 +609,6 @@ hocon_schema_to_spec(Type, LocalModule) when ?IS_TYPEREFL(Type) -> hocon_schema_to_spec(?ARRAY(Item), LocalModule) -> {Schema, Refs} = hocon_schema_to_spec(Item, LocalModule), {#{type => array, items => Schema}, Refs}; -hocon_schema_to_spec(?LAZY(Item), LocalModule) -> - hocon_schema_to_spec(Item, LocalModule); hocon_schema_to_spec(?ENUM(Items), _LocalModule) -> {#{type => string, enum => Items}, []}; hocon_schema_to_spec(?MAP(Name, Type), LocalModule) -> From c6a78cbfda4ecbbc29e6a58df861b8a5aa159e7d Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Mon, 9 Jan 2023 23:02:32 +0100 Subject: [PATCH 02/11] chore: refine a warning message about default erlang cookie --- bin/emqx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/emqx b/bin/emqx index 132d8cba7..0a967a73c 100755 --- a/bin/emqx +++ b/bin/emqx @@ -911,7 +911,7 @@ fi if [ $IS_BOOT_COMMAND = 'yes' ] && [ "$COOKIE" = "$EMQX_DEFAULT_ERLANG_COOKIE" ]; then logwarn "Default (insecure) Erlang cookie is in use." logwarn "Configure node.cookie in $EMQX_ETC_DIR/emqx.conf or override from environment variable EMQX_NODE__COOKIE" - logwarn "Use the same config value for all nodes in the cluster." + logwarn "NOTE: Use the same cookie for all nodes in the cluster." fi ## check if OTP version has mnesia_hook feature; if not, fallback to From 6aaff6211f2f7016c1e73d41850859634b0a468b Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Mon, 9 Jan 2023 09:28:03 +0100 Subject: [PATCH 03/11] feat: use union member selector for authn config schema --- apps/emqx_authn/src/emqx_authn_schema.erl | 99 +++++++++++++++++-- .../src/simple_authn/emqx_authn_http.erl | 12 ++- .../src/simple_authn/emqx_authn_jwt.erl | 15 ++- .../src/simple_authn/emqx_authn_mongodb.erl | 10 ++ .../src/simple_authn/emqx_authn_redis.erl | 10 ++ 5 files changed, 134 insertions(+), 12 deletions(-) diff --git a/apps/emqx_authn/src/emqx_authn_schema.erl b/apps/emqx_authn/src/emqx_authn_schema.erl index f40e759f0..9c4c7d89a 100644 --- a/apps/emqx_authn/src/emqx_authn_schema.erl +++ b/apps/emqx_authn/src/emqx_authn_schema.erl @@ -45,24 +45,105 @@ enable(desc) -> ?DESC(?FUNCTION_NAME); enable(_) -> undefined. authenticator_type() -> - hoconsc:union(config_refs([Module || {_AuthnType, Module} <- emqx_authn:providers()])). + hoconsc:union(union_member_selector(emqx_authn:providers())). authenticator_type_without_scram() -> Providers = lists:filtermap( fun - ({{password_based, _Backend}, Mod}) -> - {true, Mod}; - ({jwt, Mod}) -> - {true, Mod}; ({{scram, _Backend}, _Mod}) -> - false + false; + (_) -> + true end, emqx_authn:providers() ), - hoconsc:union(config_refs(Providers)). + hoconsc:union(union_member_selector(Providers)). -config_refs(Modules) -> - lists:append([Module:refs() || Module <- Modules]). +config_refs(Providers) -> + lists:append([Module:refs() || {_, Module} <- Providers]). + +union_member_selector(Providers) -> + Types = config_refs(Providers), + fun + (all_union_members) -> Types; + ({value, Value}) -> select_union_member(Value, Providers) + end. + +select_union_member(#{<<"mechanism">> := _} = Value, Providers) -> + select_union_member(Value, Providers, #{}); +select_union_member(_Value, _) -> + throw(#{hint => "missing 'mechanism' field"}). + +select_union_member(Value, [], ReasonsMap) when ReasonsMap =:= #{} -> + BackendVal = maps:get(<<"backend">>, Value, undefined), + MechanismVal = maps:get(<<"mechanism">>, Value), + throw(#{ + backend => BackendVal, + mechanism => MechanismVal, + hint => "unknown_mechanism_or_backend" + }); +select_union_member(_Value, [], ReasonsMap) -> + throw(ReasonsMap); +select_union_member(Value, [Provider | Providers], ReasonsMap) -> + {Mechanism, Backend, Module} = + case Provider of + {{M, B}, Mod} -> {atom_to_binary(M), atom_to_binary(B), Mod}; + {M, Mod} when is_atom(M) -> {atom_to_binary(M), undefined, Mod} + end, + case do_select_union_member(Mechanism, Backend, Module, Value) of + {ok, Type} -> + [Type]; + {error, nomatch} -> + %% obvious mismatch, do not complain + %% e.g. when 'backend' is "http", but the value is "redis", + %% then there is no need to record the error like + %% "'http' is exepcted but got 'redis'" + select_union_member(Value, Providers, ReasonsMap); + {error, Reason} -> + %% more interesting error message + %% e.g. when 'backend' is "http", but there is no "method" field + %% found so there is no way to tell if it's the 'get' type or 'post' type. + %% hence the error message is like: + %% #{emqx_auth_http => "'http' auth backend must have get|post as 'method'"} + select_union_member(Value, Providers, ReasonsMap#{Module => Reason}) + end. + +do_select_union_member(Mechanism, Backend, Module, Value) -> + BackendVal = maps:get(<<"backend">>, Value, undefined), + MechanismVal = maps:get(<<"mechanism">>, Value), + case MechanismVal =:= Mechanism of + true when Backend =:= undefined -> + case BackendVal =:= undefined of + true -> + %% e.g. jwt has no 'backend' + try_select_union_member(Module, Value); + false -> + {error, "unexpected 'backend' for " ++ binary_to_list(Mechanism)} + end; + true -> + case Backend =:= BackendVal of + true -> + try_select_union_member(Module, Value); + false -> + %% 'backend' not matching + {error, nomatch} + end; + false -> + %% 'mechanism' not matching + {error, nomatch} + end. + +try_select_union_member(Module, Value) -> + try + %% some modules have refs/1 exported to help selectin the sub-types + %% emqx_authn_http, emqx_authn_jwt, emqx_authn_mongodb and emqx_authn_redis + Module:refs(Value) + catch + error:undef -> + %% otherwise expect only one member from this module + [Type] = Module:refs(), + {ok, Type} + end. %% authn is a core functionality however implemented outside of emqx app %% in emqx_schema, 'authentication' is a map() type which is to allow diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl index b6b68eab9..ecd084c08 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl @@ -40,6 +40,7 @@ -export([ refs/0, + refs/1, create/2, update/2, authenticate/2, @@ -66,12 +67,12 @@ roots() -> fields(get) -> [ - {method, #{type => get, required => true, default => get, desc => ?DESC(method)}}, + {method, #{type => get, required => true, desc => ?DESC(method)}}, {headers, fun headers_no_content_type/1} ] ++ common_fields(); fields(post) -> [ - {method, #{type => post, required => true, default => post, desc => ?DESC(method)}}, + {method, #{type => post, required => true, desc => ?DESC(method)}}, {headers, fun headers/1} ] ++ common_fields(). @@ -159,6 +160,13 @@ refs() -> hoconsc:ref(?MODULE, post) ]. +refs(#{<<"method">> := <<"get">>}) -> + {ok, hoconsc:ref(?MODULE, get)}; +refs(#{<<"method">> := <<"post">>}) -> + {ok, hoconsc:ref(?MODULE, post)}; +refs(_) -> + {error, "'http' auth backend must have get|post as 'method'"}. + create(_AuthenticatorID, Config) -> create(Config). diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl index 852139875..f8a278ee5 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl @@ -21,7 +21,6 @@ -include_lib("hocon/include/hoconsc.hrl"). -behaviour(hocon_schema). --behaviour(emqx_authentication). -export([ namespace/0, @@ -33,6 +32,7 @@ -export([ refs/0, + refs/1, create/2, update/2, authenticate/2, @@ -169,6 +169,19 @@ refs() -> hoconsc:ref(?MODULE, 'jwks') ]. +refs(#{<<"mechanism">> := <<"jwt">>} = V) -> + UseJWKS = maps:get(<<"use_jwks">>, V, undefined), + select_ref(UseJWKS, V). + +select_ref(true, _) -> + {ok, hoconsc:ref(?MODULE, 'jwks')}; +select_ref(false, #{<<"public_key">> := _}) -> + {ok, hoconsc:ref(?MODULE, 'public-key')}; +select_ref(false, _) -> + {ok, hoconsc:ref(?MODULE, 'hmac-based')}; +select_ref(_, _) -> + {error, "use_jwks must be set"}. + create(_AuthenticatorID, Config) -> create(Config). diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl index bcdc7b0b1..3820005af 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl @@ -33,6 +33,7 @@ -export([ refs/0, + refs/1, create/2, update/2, authenticate/2, @@ -130,6 +131,15 @@ refs() -> hoconsc:ref(?MODULE, 'sharded-cluster') ]. +refs(#{<<"mongo_type">> := <<"single">>}) -> + {ok, hoconsc:ref(?MODULE, standalone)}; +refs(#{<<"mongo_type">> := <<"rs">>}) -> + {ok, hoconsc:ref(?MODULE, 'replica-set')}; +refs(#{<<"mongo_type">> := <<"sharded">>}) -> + {ok, hoconsc:ref(?MODULE, 'sharded-cluster')}; +refs(_) -> + {error, "unknown 'mongo_type'"}. + create(_AuthenticatorID, Config) -> create(Config). diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl index 8c9cb5333..cafcf1830 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl @@ -33,6 +33,7 @@ -export([ refs/0, + refs/1, create/2, update/2, authenticate/2, @@ -97,6 +98,15 @@ refs() -> hoconsc:ref(?MODULE, sentinel) ]. +refs(#{<<"redis_type">> := <<"single">>}) -> + {ok, hoconsc:ref(?MODULE, standalone)}; +refs(#{<<"redis_type">> := <<"cluster">>}) -> + {ok, hoconsc:ref(?MODULE, cluster)}; +refs(#{<<"redis_type">> := <<"sentinel">>}) -> + {ok, hoconsc:ref(?MODULE, sentinel)}; +refs(_) -> + {error, "unknown 'redis_type'"}. + create(_AuthenticatorID, Config) -> create(Config). From fbc52330f3c42af47f73489ab702cd7909acb4e3 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Wed, 11 Jan 2023 15:20:39 +0100 Subject: [PATCH 04/11] refactor: do not re-check config in post_config_update now that the type is not lazy, it should have already been checked when reaching the post_config_update step --- apps/emqx/src/emqx_authentication_config.erl | 52 +------------------- apps/emqx_authn/src/emqx_authn_app.erl | 6 ++- 2 files changed, 5 insertions(+), 53 deletions(-) diff --git a/apps/emqx/src/emqx_authentication_config.erl b/apps/emqx/src/emqx_authentication_config.erl index 300fb4a66..5471b66fc 100644 --- a/apps/emqx/src/emqx_authentication_config.erl +++ b/apps/emqx/src/emqx_authentication_config.erl @@ -136,7 +136,7 @@ do_pre_config_update({move_authenticator, _ChainName, AuthenticatorID, Position} ) -> ok | {ok, map()} | {error, term()}. post_config_update(_, UpdateReq, NewConfig, OldConfig, AppEnvs) -> - do_post_config_update(UpdateReq, check_configs(to_list(NewConfig)), OldConfig, AppEnvs). + do_post_config_update(UpdateReq, to_list(NewConfig), OldConfig, AppEnvs). do_post_config_update({create_authenticator, ChainName, Config}, NewConfig, _OldConfig, _AppEnvs) -> NConfig = get_authenticator_config(authenticator_id(Config), NewConfig), @@ -175,56 +175,6 @@ do_post_config_update( ) -> emqx_authentication:move_authenticator(ChainName, AuthenticatorID, Position). -check_configs(Configs) -> - Providers = emqx_authentication:get_providers(), - lists:map(fun(C) -> do_check_config(C, Providers) end, Configs). - -do_check_config(Config, Providers) -> - Type = authn_type(Config), - case maps:get(Type, Providers, false) of - false -> - ?SLOG(warning, #{ - msg => "unknown_authn_type", - type => Type, - providers => Providers - }), - throw({unknown_authn_type, Type}); - Module -> - do_check_config(Type, Config, Module) - end. - -do_check_config(Type, Config, Module) -> - F = - case erlang:function_exported(Module, check_config, 1) of - true -> - fun Module:check_config/1; - false -> - fun(C) -> - Key = list_to_binary(?EMQX_AUTHENTICATION_CONFIG_ROOT_NAME), - AtomKey = list_to_atom(?EMQX_AUTHENTICATION_CONFIG_ROOT_NAME), - R = hocon_tconf:check_plain( - Module, - #{Key => C}, - #{atom_key => true} - ), - maps:get(AtomKey, R) - end - end, - try - F(Config) - catch - C:E:S -> - ?SLOG(warning, #{ - msg => "failed_to_check_config", - config => Config, - type => Type, - exception => C, - reason => E, - stacktrace => S - }), - throw({bad_authenticator_config, #{type => Type, reason => E}}) - end. - to_list(undefined) -> []; to_list(M) when M =:= #{} -> []; to_list(M) when is_map(M) -> [M]; diff --git a/apps/emqx_authn/src/emqx_authn_app.erl b/apps/emqx_authn/src/emqx_authn_app.erl index 99a141822..ed97c0975 100644 --- a/apps/emqx_authn/src/emqx_authn_app.erl +++ b/apps/emqx_authn/src/emqx_authn_app.erl @@ -35,6 +35,9 @@ %%------------------------------------------------------------------------------ start(_StartType, _StartArgs) -> + %% required by test cases, ensure the injection of + %% EMQX_AUTHENTICATION_SCHEMA_MODULE_PT_KEY + _ = emqx_conf_schema:roots(), ok = mria_rlog:wait_for_shards([?AUTH_SHARD], infinity), {ok, Sup} = emqx_authn_sup:start_link(), case initialize() of @@ -43,8 +46,7 @@ start(_StartType, _StartArgs) -> end. stop(_State) -> - ok = deinitialize(), - ok. + ok = deinitialize(). %%------------------------------------------------------------------------------ %% Internal functions From 65319567cc242701d294425c37589192c2995cdb Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Wed, 11 Jan 2023 20:00:28 +0100 Subject: [PATCH 05/11] chore: no type check for 3rd party auth --- apps/emqx/src/emqx_authentication.erl | 5 +- apps/emqx/test/emqx_authentication_SUITE.erl | 54 +++----------------- 2 files changed, 10 insertions(+), 49 deletions(-) diff --git a/apps/emqx/src/emqx_authentication.erl b/apps/emqx/src/emqx_authentication.erl index ec8a1c113..fe93bed68 100644 --- a/apps/emqx/src/emqx_authentication.erl +++ b/apps/emqx/src/emqx_authentication.erl @@ -759,9 +759,10 @@ maybe_unhook(State) -> State. do_create_authenticator(AuthenticatorID, #{enable := Enable} = Config, Providers) -> - case maps:get(authn_type(Config), Providers, undefined) of + Type = authn_type(Config), + case maps:get(Type, Providers, undefined) of undefined -> - {error, no_available_provider}; + {error, {no_available_provider_for, Type}}; Provider -> case Provider:create(AuthenticatorID, Config) of {ok, State} -> diff --git a/apps/emqx/test/emqx_authentication_SUITE.erl b/apps/emqx/test/emqx_authentication_SUITE.erl index 77789dedd..2c83162ed 100644 --- a/apps/emqx/test/emqx_authentication_SUITE.erl +++ b/apps/emqx/test/emqx_authentication_SUITE.erl @@ -52,50 +52,10 @@ ) ). -%%------------------------------------------------------------------------------ -%% Hocon Schema -%%------------------------------------------------------------------------------ - -roots() -> - [ - {config, #{ - type => hoconsc:union([ - hoconsc:ref(?MODULE, type1), - hoconsc:ref(?MODULE, type2) - ]) - }} - ]. - -fields(type1) -> - [ - {mechanism, {enum, [password_based]}}, - {backend, {enum, [built_in_database]}}, - {enable, fun enable/1} - ]; -fields(type2) -> - [ - {mechanism, {enum, [password_based]}}, - {backend, {enum, [mysql]}}, - {enable, fun enable/1} - ]. - -enable(type) -> boolean(); -enable(default) -> true; -enable(_) -> undefined. - %%------------------------------------------------------------------------------ %% Callbacks %%------------------------------------------------------------------------------ -check_config(C) -> - #{config := R} = - hocon_tconf:check_plain( - ?MODULE, - #{<<"config">> => C}, - #{atom_key => true} - ), - R. - create(_AuthenticatorID, _Config) -> {ok, #{mark => 1}}. @@ -200,7 +160,7 @@ t_authenticator(Config) when is_list(Config) -> % Create an authenticator when the provider does not exist ?assertEqual( - {error, no_available_provider}, + {error, {no_available_provider_for, {password_based, built_in_database}}}, ?AUTHN:create_authenticator(ChainName, AuthenticatorConfig1) ), @@ -335,14 +295,14 @@ t_update_config(Config) when is_list(Config) -> ok = register_provider(?config("auth2"), ?MODULE), Global = ?config(global), AuthenticatorConfig1 = #{ - <<"mechanism">> => <<"password_based">>, - <<"backend">> => <<"built_in_database">>, - <<"enable">> => true + mechanism => password_based, + backend => built_in_database, + enable => true }, AuthenticatorConfig2 = #{ - <<"mechanism">> => <<"password_based">>, - <<"backend">> => <<"mysql">>, - <<"enable">> => true + mechanism => password_based, + backend => mysql, + enable => true }, ID1 = <<"password_based:built_in_database">>, ID2 = <<"password_based:mysql">>, From 4f91bf415c68688e4b4f8359b1e7fcd1e8014309 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Thu, 12 Jan 2023 12:58:28 +0100 Subject: [PATCH 06/11] fix: schema check error reason pattern --- apps/emqx/src/emqx_schema.erl | 7 +++- apps/emqx_authn/src/emqx_authn_api.erl | 7 +--- .../src/emqx_authn_password_hashing.erl | 34 +++++++++++++------ .../src/simple_authn/emqx_authn_jwt.erl | 11 ++++-- .../test/emqx_authn_mnesia_SUITE.erl | 32 +++++++++++++---- .../test/emqx_authn_pgsql_SUITE.erl | 17 ++++------ .../test/emqx_authn_redis_SUITE.erl | 10 +++--- 7 files changed, 78 insertions(+), 40 deletions(-) diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl index 77b51f074..fa4b068a1 100644 --- a/apps/emqx/src/emqx_schema.erl +++ b/apps/emqx/src/emqx_schema.erl @@ -2363,7 +2363,12 @@ authentication(Which) -> Module -> Module:root_type() end, - hoconsc:mk(Type, #{desc => Desc}). + hoconsc:mk(Type, #{desc => Desc, converter => fun ensure_array/1}). + +%% the older version schema allows individual element (instead of a chain) in config +ensure_array(undefined) -> undefined; +ensure_array(L) when is_list(L) -> L; +ensure_array(M) when is_map(M) -> [M]. -spec qos() -> typerefl:type(). qos() -> diff --git a/apps/emqx_authn/src/emqx_authn_api.erl b/apps/emqx_authn/src/emqx_authn_api.erl index 452a7bb90..7e1c613d3 100644 --- a/apps/emqx_authn/src/emqx_authn_api.erl +++ b/apps/emqx_authn/src/emqx_authn_api.erl @@ -1232,15 +1232,10 @@ serialize_error({unknown_authn_type, Type}) -> code => <<"BAD_REQUEST">>, message => binfmt("Unknown type '~p'", [Type]) }}; -serialize_error({bad_authenticator_config, Reason}) -> - {400, #{ - code => <<"BAD_REQUEST">>, - message => binfmt("Bad authenticator config ~p", [Reason]) - }}; serialize_error(Reason) -> {400, #{ code => <<"BAD_REQUEST">>, - message => binfmt("~p", [Reason]) + message => binfmt("~0p", [Reason]) }}. parse_position(<<"front">>) -> diff --git a/apps/emqx_authn/src/emqx_authn_password_hashing.erl b/apps/emqx_authn/src/emqx_authn_password_hashing.erl index b3e90e2cd..4954cd66e 100644 --- a/apps/emqx_authn/src/emqx_authn_password_hashing.erl +++ b/apps/emqx_authn/src/emqx_authn_password_hashing.erl @@ -64,7 +64,7 @@ ]). namespace() -> "authn-hash". -roots() -> [pbkdf2, bcrypt, bcrypt_rw, other_algorithms]. +roots() -> [pbkdf2, bcrypt, bcrypt_rw, simple]. fields(bcrypt_rw) -> fields(bcrypt) ++ @@ -96,7 +96,7 @@ fields(pbkdf2) -> )}, {dk_length, fun dk_length/1} ]; -fields(other_algorithms) -> +fields(simple) -> [ {name, sc( @@ -112,8 +112,8 @@ desc(bcrypt) -> "Settings for bcrypt password hashing algorithm."; desc(pbkdf2) -> "Settings for PBKDF2 password hashing algorithm."; -desc(other_algorithms) -> - "Settings for other password hashing algorithms."; +desc(simple) -> + "Settings for simple algorithms."; desc(_) -> undefined. @@ -231,17 +231,31 @@ check_password(#{name := Other, salt_position := SaltPosition}, Salt, PasswordHa %%------------------------------------------------------------------------------ rw_refs() -> - [ + All = [ hoconsc:ref(?MODULE, bcrypt_rw), hoconsc:ref(?MODULE, pbkdf2), - hoconsc:ref(?MODULE, other_algorithms) - ]. + hoconsc:ref(?MODULE, simple) + ], + fun + (all_union_members) -> All; + ({value, #{<<"name">> := <<"bcrypt">>}}) -> [hoconsc:ref(?MODULE, bcrypt_rw)]; + ({value, #{<<"name">> := <<"pbkdf2">>}}) -> [hoconsc:ref(?MODULE, pbkdf2)]; + ({value, #{<<"name">> := _}}) -> [hoconsc:ref(?MODULE, simple)]; + ({value, _}) -> throw(#{reason => "algorithm_name_missing"}) + end. ro_refs() -> - [ + All = [ hoconsc:ref(?MODULE, bcrypt), hoconsc:ref(?MODULE, pbkdf2), - hoconsc:ref(?MODULE, other_algorithms) - ]. + hoconsc:ref(?MODULE, simple) + ], + fun + (all_union_members) -> All; + ({value, #{<<"name">> := <<"bcrypt">>}}) -> [hoconsc:ref(?MODULE, bcrypt)]; + ({value, #{<<"name">> := <<"pbkdf2">>}}) -> [hoconsc:ref(?MODULE, pbkdf2)]; + ({value, #{<<"name">> := _}}) -> [hoconsc:ref(?MODULE, simple)]; + ({value, _}) -> throw(#{reason => "algorithm_name_missing"}) + end. sc(Type, Meta) -> hoconsc:mk(Type, Meta). diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl index f8a278ee5..e32786b04 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl @@ -171,7 +171,14 @@ refs() -> refs(#{<<"mechanism">> := <<"jwt">>} = V) -> UseJWKS = maps:get(<<"use_jwks">>, V, undefined), - select_ref(UseJWKS, V). + select_ref(boolean(UseJWKS), V). + +%% this field is technically a boolean type, +%% but union member selection is done before type casting (by typrefl), +%% so we have to allow strings too +boolean(<<"true">>) -> true; +boolean(<<"false">>) -> false; +boolean(Other) -> Other. select_ref(true, _) -> {ok, hoconsc:ref(?MODULE, 'jwks')}; @@ -180,7 +187,7 @@ select_ref(false, #{<<"public_key">> := _}) -> select_ref(false, _) -> {ok, hoconsc:ref(?MODULE, 'hmac-based')}; select_ref(_, _) -> - {error, "use_jwks must be set"}. + {error, "use_jwks must be set to true or false"}. create(_AuthenticatorID, Config) -> create(Config). diff --git a/apps/emqx_authn/test/emqx_authn_mnesia_SUITE.erl b/apps/emqx_authn/test/emqx_authn_mnesia_SUITE.erl index cd97a15d9..740e7387a 100644 --- a/apps/emqx_authn/test/emqx_authn_mnesia_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_mnesia_SUITE.erl @@ -52,6 +52,7 @@ end_per_testcase(_Case, Config) -> -define(CONF(Conf), #{?CONF_NS_BINARY => Conf}). t_check_schema(_Config) -> + Check = fun(C) -> emqx_config:check_config(emqx_schema, ?CONF(C)) end, ConfigOk = #{ <<"mechanism">> => <<"password_based">>, <<"backend">> => <<"built_in_database">>, @@ -61,8 +62,7 @@ t_check_schema(_Config) -> <<"salt_rounds">> => <<"6">> } }, - - hocon_tconf:check_plain(emqx_authn_mnesia, ?CONF(ConfigOk)), + _ = Check(ConfigOk), ConfigNotOk = #{ <<"mechanism">> => <<"password_based">>, @@ -72,11 +72,31 @@ t_check_schema(_Config) -> <<"name">> => <<"md6">> } }, + ?assertThrow( + #{ + path := "authentication.1.password_hash_algorithm.name", + matched_type := "authn-builtin_db:authentication/authn-hash:simple", + reason := unable_to_convert_to_enum_symbol + }, + Check(ConfigNotOk) + ), - ?assertException( - throw, - {emqx_authn_mnesia, _}, - hocon_tconf:check_plain(emqx_authn_mnesia, ?CONF(ConfigNotOk)) + ConfigMissingAlgoName = #{ + <<"mechanism">> => <<"password_based">>, + <<"backend">> => <<"built_in_database">>, + <<"user_id_type">> => <<"username">>, + <<"password_hash_algorithm">> => #{ + <<"foo">> => <<"bar">> + } + }, + + ?assertThrow( + #{ + path := "authentication.1.password_hash_algorithm", + reason := "algorithm_name_missing", + matched_type := "authn-builtin_db:authentication" + }, + Check(ConfigMissingAlgoName) ). t_create(_) -> diff --git a/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl b/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl index fa0658f6a..65c783298 100644 --- a/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl @@ -105,17 +105,12 @@ t_update_with_invalid_config(_Config) -> AuthConfig = raw_pgsql_auth_config(), BadConfig = maps:without([<<"server">>], AuthConfig), ?assertMatch( - {error, - {bad_authenticator_config, #{ - reason := - {emqx_authn_pgsql, [ - #{ - kind := validation_error, - path := "authentication.server", - reason := required_field - } - ]} - }}}, + {error, #{ + kind := validation_error, + matched_type := "authn-postgresql:authentication", + path := "authentication.1.server", + reason := required_field + }}, emqx:update_config( ?PATH, {create_authenticator, ?GLOBAL, BadConfig} diff --git a/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl b/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl index 4f25c8709..4f95bef93 100644 --- a/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl @@ -160,10 +160,12 @@ t_create_invalid_config(_Config) -> Config0 = raw_redis_auth_config(), Config = maps:without([<<"server">>], Config0), ?assertMatch( - {error, - {bad_authenticator_config, #{ - reason := {emqx_authn_redis, [#{kind := validation_error}]} - }}}, + {error, #{ + kind := validation_error, + matched_type := "authn-redis:standalone", + path := "authentication.1.server", + reason := required_field + }}, emqx:update_config(?PATH, {create_authenticator, ?GLOBAL, Config}) ), ?assertMatch([], emqx_config:get_raw([authentication])), From 2ebc89e339d6c4294d62553ebe9f1f499964fdb1 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Fri, 13 Jan 2023 14:05:57 +0100 Subject: [PATCH 07/11] refactor: authn schema union selector --- apps/emqx/src/emqx_schema.erl | 8 +- apps/emqx_authn/src/emqx_authn_schema.erl | 107 ++++------ .../src/simple_authn/emqx_authn_http.erl | 5 +- .../src/simple_authn/emqx_authn_jwt.erl | 5 +- .../src/simple_authn/emqx_authn_mongodb.erl | 5 +- .../src/simple_authn/emqx_authn_redis.erl | 5 +- .../test/emqx_authn_mnesia_SUITE.erl | 50 ----- .../test/emqx_authn_schema_SUITE.erl | 190 ++++++++++++++++++ 8 files changed, 250 insertions(+), 125 deletions(-) create mode 100644 apps/emqx_authn/test/emqx_authn_schema_SUITE.erl diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl index fa4b068a1..ba813d2c8 100644 --- a/apps/emqx/src/emqx_schema.erl +++ b/apps/emqx/src/emqx_schema.erl @@ -2363,12 +2363,12 @@ authentication(Which) -> Module -> Module:root_type() end, - hoconsc:mk(Type, #{desc => Desc, converter => fun ensure_array/1}). + hoconsc:mk(Type, #{desc => Desc, converter => fun ensure_array/2}). %% the older version schema allows individual element (instead of a chain) in config -ensure_array(undefined) -> undefined; -ensure_array(L) when is_list(L) -> L; -ensure_array(M) when is_map(M) -> [M]. +ensure_array(undefined, _) -> undefined; +ensure_array(L, _) when is_list(L) -> L; +ensure_array(M, _) -> [M]. -spec qos() -> typerefl:type(). qos() -> diff --git a/apps/emqx_authn/src/emqx_authn_schema.erl b/apps/emqx_authn/src/emqx_authn_schema.erl index 9c4c7d89a..ce90aebe9 100644 --- a/apps/emqx_authn/src/emqx_authn_schema.erl +++ b/apps/emqx_authn/src/emqx_authn_schema.erl @@ -69,80 +69,53 @@ union_member_selector(Providers) -> ({value, Value}) -> select_union_member(Value, Providers) end. -select_union_member(#{<<"mechanism">> := _} = Value, Providers) -> - select_union_member(Value, Providers, #{}); -select_union_member(_Value, _) -> - throw(#{hint => "missing 'mechanism' field"}). - -select_union_member(Value, [], ReasonsMap) when ReasonsMap =:= #{} -> +select_union_member(#{<<"mechanism">> := _} = Value, Providers0) -> BackendVal = maps:get(<<"backend">>, Value, undefined), MechanismVal = maps:get(<<"mechanism">>, Value), - throw(#{ - backend => BackendVal, - mechanism => MechanismVal, - hint => "unknown_mechanism_or_backend" - }); -select_union_member(_Value, [], ReasonsMap) -> - throw(ReasonsMap); -select_union_member(Value, [Provider | Providers], ReasonsMap) -> - {Mechanism, Backend, Module} = - case Provider of - {{M, B}, Mod} -> {atom_to_binary(M), atom_to_binary(B), Mod}; - {M, Mod} when is_atom(M) -> {atom_to_binary(M), undefined, Mod} - end, - case do_select_union_member(Mechanism, Backend, Module, Value) of - {ok, Type} -> - [Type]; - {error, nomatch} -> - %% obvious mismatch, do not complain - %% e.g. when 'backend' is "http", but the value is "redis", - %% then there is no need to record the error like - %% "'http' is exepcted but got 'redis'" - select_union_member(Value, Providers, ReasonsMap); - {error, Reason} -> - %% more interesting error message - %% e.g. when 'backend' is "http", but there is no "method" field - %% found so there is no way to tell if it's the 'get' type or 'post' type. - %% hence the error message is like: - %% #{emqx_auth_http => "'http' auth backend must have get|post as 'method'"} - select_union_member(Value, Providers, ReasonsMap#{Module => Reason}) - end. - -do_select_union_member(Mechanism, Backend, Module, Value) -> - BackendVal = maps:get(<<"backend">>, Value, undefined), - MechanismVal = maps:get(<<"mechanism">>, Value), - case MechanismVal =:= Mechanism of - true when Backend =:= undefined -> - case BackendVal =:= undefined of - true -> - %% e.g. jwt has no 'backend' - try_select_union_member(Module, Value); - false -> - {error, "unexpected 'backend' for " ++ binary_to_list(Mechanism)} - end; - true -> - case Backend =:= BackendVal of - true -> - try_select_union_member(Module, Value); - false -> - %% 'backend' not matching - {error, nomatch} - end; - false -> - %% 'mechanism' not matching - {error, nomatch} - end. + BackendFilterFn = fun + ({{_Mec, Backend}, _Mod}) -> + BackendVal =:= atom_to_binary(Backend); + (_) -> + BackendVal =:= undefined + end, + MechanismFilterFn = fun + ({{Mechanism, _Backend}, _Mod}) -> + MechanismVal =:= atom_to_binary(Mechanism); + ({Mechanism, _Mod}) -> + MechanismVal =:= atom_to_binary(Mechanism) + end, + case lists:filter(BackendFilterFn, Providers0) of + [] -> + throw(#{reason => "unknown_backend", backend => BackendVal}); + Providers1 -> + case lists:filter(MechanismFilterFn, Providers1) of + [] -> + throw(#{ + reason => "unsupported_mechanism", + mechanism => MechanismVal, + backend => BackendVal + }); + [{_, Module}] -> + try_select_union_member(Module, Value) + end + end; +select_union_member(Value, _Providers) when is_map(Value) -> + throw(#{reason => "missing_mechanism_field"}); +select_union_member(Value, _Providers) -> + throw(#{reason => "not_a_struct", value => Value}). try_select_union_member(Module, Value) -> - try - %% some modules have refs/1 exported to help selectin the sub-types - %% emqx_authn_http, emqx_authn_jwt, emqx_authn_mongodb and emqx_authn_redis - Module:refs(Value) + %% some modules have refs/1 exported to help selectin the sub-types + %% emqx_authn_http, emqx_authn_jwt, emqx_authn_mongodb and emqx_authn_redis + try Module:refs(Value) of + {ok, Type} -> + [Type]; + {error, Reason} -> + throw(Reason) catch error:undef -> %% otherwise expect only one member from this module - [Type] = Module:refs(), - {ok, Type} + Module:refs() end. %% authn is a core functionality however implemented outside of emqx app diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl index ecd084c08..0c0ec3b06 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl @@ -165,7 +165,10 @@ refs(#{<<"method">> := <<"get">>}) -> refs(#{<<"method">> := <<"post">>}) -> {ok, hoconsc:ref(?MODULE, post)}; refs(_) -> - {error, "'http' auth backend must have get|post as 'method'"}. + {error, #{ + field_name => method, + expected => "get | post" + }}. create(_AuthenticatorID, Config) -> create(Config). diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl index e32786b04..d96f9073e 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl @@ -187,7 +187,10 @@ select_ref(false, #{<<"public_key">> := _}) -> select_ref(false, _) -> {ok, hoconsc:ref(?MODULE, 'hmac-based')}; select_ref(_, _) -> - {error, "use_jwks must be set to true or false"}. + {error, #{ + field_name => use_jwks, + expected => "true | false" + }}. create(_AuthenticatorID, Config) -> create(Config). diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl index 3820005af..11048528c 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl @@ -138,7 +138,10 @@ refs(#{<<"mongo_type">> := <<"rs">>}) -> refs(#{<<"mongo_type">> := <<"sharded">>}) -> {ok, hoconsc:ref(?MODULE, 'sharded-cluster')}; refs(_) -> - {error, "unknown 'mongo_type'"}. + {error, #{ + field_name => mongo_type, + expected => "single | rs | sharded" + }}. create(_AuthenticatorID, Config) -> create(Config). diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl index cafcf1830..419899bbd 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl @@ -105,7 +105,10 @@ refs(#{<<"redis_type">> := <<"cluster">>}) -> refs(#{<<"redis_type">> := <<"sentinel">>}) -> {ok, hoconsc:ref(?MODULE, sentinel)}; refs(_) -> - {error, "unknown 'redis_type'"}. + {error, #{ + field_name => redis_type, + expected => "single | cluster | sentinel" + }}. create(_AuthenticatorID, Config) -> create(Config). diff --git a/apps/emqx_authn/test/emqx_authn_mnesia_SUITE.erl b/apps/emqx_authn/test/emqx_authn_mnesia_SUITE.erl index 740e7387a..599eae92e 100644 --- a/apps/emqx_authn/test/emqx_authn_mnesia_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_mnesia_SUITE.erl @@ -49,56 +49,6 @@ end_per_testcase(_Case, Config) -> %% Tests %%------------------------------------------------------------------------------ --define(CONF(Conf), #{?CONF_NS_BINARY => Conf}). - -t_check_schema(_Config) -> - Check = fun(C) -> emqx_config:check_config(emqx_schema, ?CONF(C)) end, - ConfigOk = #{ - <<"mechanism">> => <<"password_based">>, - <<"backend">> => <<"built_in_database">>, - <<"user_id_type">> => <<"username">>, - <<"password_hash_algorithm">> => #{ - <<"name">> => <<"bcrypt">>, - <<"salt_rounds">> => <<"6">> - } - }, - _ = Check(ConfigOk), - - ConfigNotOk = #{ - <<"mechanism">> => <<"password_based">>, - <<"backend">> => <<"built_in_database">>, - <<"user_id_type">> => <<"username">>, - <<"password_hash_algorithm">> => #{ - <<"name">> => <<"md6">> - } - }, - ?assertThrow( - #{ - path := "authentication.1.password_hash_algorithm.name", - matched_type := "authn-builtin_db:authentication/authn-hash:simple", - reason := unable_to_convert_to_enum_symbol - }, - Check(ConfigNotOk) - ), - - ConfigMissingAlgoName = #{ - <<"mechanism">> => <<"password_based">>, - <<"backend">> => <<"built_in_database">>, - <<"user_id_type">> => <<"username">>, - <<"password_hash_algorithm">> => #{ - <<"foo">> => <<"bar">> - } - }, - - ?assertThrow( - #{ - path := "authentication.1.password_hash_algorithm", - reason := "algorithm_name_missing", - matched_type := "authn-builtin_db:authentication" - }, - Check(ConfigMissingAlgoName) - ). - t_create(_) -> Config0 = config(), diff --git a/apps/emqx_authn/test/emqx_authn_schema_SUITE.erl b/apps/emqx_authn/test/emqx_authn_schema_SUITE.erl new file mode 100644 index 000000000..7e67584ac --- /dev/null +++ b/apps/emqx_authn/test/emqx_authn_schema_SUITE.erl @@ -0,0 +1,190 @@ +-module(emqx_authn_schema_SUITE). + +-compile(export_all). +-compile(nowarn_export_all). + +-include_lib("eunit/include/eunit.hrl"). + +-include("emqx_authn.hrl"). + +all() -> + emqx_common_test_helpers:all(?MODULE). + +init_per_suite(Config) -> + _ = application:load(emqx_conf), + emqx_common_test_helpers:start_apps([emqx_authn]), + Config. + +end_per_suite(_) -> + emqx_common_test_helpers:stop_apps([emqx_authn]), + ok. + +init_per_testcase(_Case, Config) -> + {ok, _} = emqx_cluster_rpc:start_link(node(), emqx_cluster_rpc, 1000), + mria:clear_table(emqx_authn_mnesia), + Config. + +end_per_testcase(_Case, Config) -> + Config. + +-define(CONF(Conf), #{?CONF_NS_BINARY => Conf}). + +t_check_schema(_Config) -> + Check = fun(C) -> emqx_config:check_config(emqx_schema, ?CONF(C)) end, + ConfigOk = #{ + <<"mechanism">> => <<"password_based">>, + <<"backend">> => <<"built_in_database">>, + <<"user_id_type">> => <<"username">>, + <<"password_hash_algorithm">> => #{ + <<"name">> => <<"bcrypt">>, + <<"salt_rounds">> => <<"6">> + } + }, + _ = Check(ConfigOk), + + ConfigNotOk = #{ + <<"mechanism">> => <<"password_based">>, + <<"backend">> => <<"built_in_database">>, + <<"user_id_type">> => <<"username">>, + <<"password_hash_algorithm">> => #{ + <<"name">> => <<"md6">> + } + }, + ?assertThrow( + #{ + path := "authentication.1.password_hash_algorithm.name", + matched_type := "authn-builtin_db:authentication/authn-hash:simple", + reason := unable_to_convert_to_enum_symbol + }, + Check(ConfigNotOk) + ), + + ConfigMissingAlgoName = #{ + <<"mechanism">> => <<"password_based">>, + <<"backend">> => <<"built_in_database">>, + <<"user_id_type">> => <<"username">>, + <<"password_hash_algorithm">> => #{ + <<"foo">> => <<"bar">> + } + }, + + ?assertThrow( + #{ + path := "authentication.1.password_hash_algorithm", + reason := "algorithm_name_missing", + matched_type := "authn-builtin_db:authentication" + }, + Check(ConfigMissingAlgoName) + ). + +t_union_member_selector(_) -> + ?assertMatch(#{authentication := undefined}, check(undefined)), + C1 = #{<<"backend">> => <<"built_in_database">>}, + ?assertThrow( + #{ + path := "authentication.1", + reason := "missing_mechanism_field" + }, + check(C1) + ), + C2 = <<"foobar">>, + ?assertThrow( + #{ + path := "authentication.1", + reason := "not_a_struct", + value := <<"foobar">> + }, + check(C2) + ), + Base = #{ + <<"user_id_type">> => <<"username">>, + <<"password_hash_algorithm">> => #{ + <<"name">> => <<"plain">> + } + }, + BadBackend = Base#{<<"mechanism">> => <<"password_based">>, <<"backend">> => <<"bar">>}, + ?assertThrow( + #{ + reason := "unknown_backend", + backend := <<"bar">> + }, + check(BadBackend) + ), + BadMechanism = Base#{<<"mechanism">> => <<"foo">>, <<"backend">> => <<"built_in_database">>}, + ?assertThrow( + #{ + reason := "unsupported_mechanism", + mechanism := <<"foo">>, + backend := <<"built_in_database">> + }, + check(BadMechanism) + ), + BadCombination = Base#{<<"mechanism">> => <<"scram">>, <<"backend">> => <<"http">>}, + ?assertThrow( + #{ + reason := "unsupported_mechanism", + mechanism := <<"scram">>, + backend := <<"http">> + }, + check(BadCombination) + ), + ok. + +t_http_auth_selector(_) -> + C1 = #{ + <<"mechanism">> => <<"password_based">>, + <<"backend">> => <<"http">> + }, + ?assertThrow( + #{ + field_name := method, + expected := "get | post" + }, + check(C1) + ), + ok. + +t_mongo_auth_selector(_) -> + C1 = #{ + <<"mechanism">> => <<"password_based">>, + <<"backend">> => <<"mongodb">> + }, + ?assertThrow( + #{ + field_name := mongo_type, + expected := "single | rs | sharded" + }, + check(C1) + ), + ok. + +t_redis_auth_selector(_) -> + C1 = #{ + <<"mechanism">> => <<"password_based">>, + <<"backend">> => <<"redis">> + }, + ?assertThrow( + #{ + field_name := redis_type, + expected := "single | cluster | sentinel" + }, + check(C1) + ), + ok. + +t_redis_jwt_selector(_) -> + C1 = #{ + <<"mechanism">> => <<"jwt">> + }, + ?assertThrow( + #{ + field_name := use_jwks, + expected := "true | false" + }, + check(C1) + ), + ok. + +check(C) -> + {_Mappings, Checked} = emqx_config:check_config(emqx_schema, ?CONF(C)), + Checked. From a9c650da66a7f02af34bc8d14a73009c3ba33bae Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Wed, 18 Jan 2023 08:05:40 +0100 Subject: [PATCH 08/11] refactor: for better test coverage --- apps/emqx/src/emqx_config.erl | 8 +- apps/emqx/src/emqx_hocon.erl | 36 ++++- apps/emqx_authn/src/emqx_authn.erl | 2 +- apps/emqx_authn/src/emqx_authn_schema.erl | 9 +- .../src/simple_authn/emqx_authn_http.erl | 17 ++- .../src/simple_authn/emqx_authn_jwt.erl | 18 +-- .../src/simple_authn/emqx_authn_mongodb.erl | 33 +++-- .../src/simple_authn/emqx_authn_redis.erl | 19 ++- .../test/emqx_authn_schema_tests.erl | 135 ++++++++++++++++++ 9 files changed, 229 insertions(+), 48 deletions(-) create mode 100644 apps/emqx_authn/test/emqx_authn_schema_tests.erl diff --git a/apps/emqx/src/emqx_config.erl b/apps/emqx/src/emqx_config.erl index c462b6725..fb84d64a1 100644 --- a/apps/emqx/src/emqx_config.erl +++ b/apps/emqx/src/emqx_config.erl @@ -413,8 +413,8 @@ check_config(SchemaMod, RawConf, Opts0) -> try do_check_config(SchemaMod, RawConf, Opts0) catch - throw:{Schema, Errors} -> - compact_errors(Schema, Errors) + throw:Errors:Stacktrace -> + throw(emqx_hocon:compact_errors(Errors, Stacktrace)) end. %% HOCON tries to be very informative about all the detailed errors @@ -425,8 +425,8 @@ compact_errors(Schema, [Error0 | More]) when is_map(Error0) -> case length(More) of 0 -> Error0; - _ -> - Error0#{unshown_errors => length(More)} + N -> + Error0#{unshown_errors => N} end, Error = case is_atom(Schema) of diff --git a/apps/emqx/src/emqx_hocon.erl b/apps/emqx/src/emqx_hocon.erl index 4391a9a0b..a039aa8c2 100644 --- a/apps/emqx/src/emqx_hocon.erl +++ b/apps/emqx/src/emqx_hocon.erl @@ -20,6 +20,7 @@ -export([ format_path/1, check/2, + compact_errors/2, format_error/1, format_error/2, make_schema/1 @@ -43,8 +44,8 @@ check(SchemaModule, Conf) when is_map(Conf) -> try {ok, hocon_tconf:check_plain(SchemaModule, Conf, Opts)} catch - throw:Reason -> - {error, Reason} + throw:Errors:Stacktrace -> + compact_errors(Errors, Stacktrace) end; check(SchemaModule, HoconText) -> case hocon:binary(HoconText, #{format => map}) of @@ -90,3 +91,34 @@ iol(L) when is_list(L) -> L. no_stacktrace(Map) -> maps:without([stacktrace], Map). + +%% @doc HOCON tries to be very informative about all the detailed errors +%% it's maybe too much when reporting to the user +-spec compact_errors(any(), Stacktrace :: list()) -> {error, any()}. +compact_errors({SchemaModule, Errors}, Stacktrace) -> + compact_errors(SchemaModule, Errors, Stacktrace). + +compact_errors(SchemaModule, [Error0 | More], _Stacktrace) when is_map(Error0) -> + Error1 = + case length(More) of + 0 -> + Error0; + N -> + Error0#{unshown_errors_count => N} + end, + Error = + case is_atom(SchemaModule) of + true -> + Error1#{schema_module => SchemaModule}; + false -> + Error1 + end, + {error, Error}; +compact_errors(SchemaModule, Error, Stacktrace) -> + %% unexpected, we need the stacktrace reported, hence error + %% if this happens i'ts a bug in hocon_tconf + {error, #{ + schema_module => SchemaModule, + exception => Error, + stacktrace => Stacktrace + }}. diff --git a/apps/emqx_authn/src/emqx_authn.erl b/apps/emqx_authn/src/emqx_authn.erl index 8c8e2efd9..934c57fb1 100644 --- a/apps/emqx_authn/src/emqx_authn.erl +++ b/apps/emqx_authn/src/emqx_authn.erl @@ -69,7 +69,7 @@ do_check_config(#{<<"mechanism">> := Mec0} = Config, Opts) -> false -> throw(#{error => unknown_authn_provider, which => Key}); {_, ProviderModule} -> - hocon_tconf:check_plain( + emqx_hocon:check( ProviderModule, #{?CONF_NS_BINARY => Config}, Opts#{atom_key => true} diff --git a/apps/emqx_authn/src/emqx_authn_schema.erl b/apps/emqx_authn/src/emqx_authn_schema.erl index ce90aebe9..f4a966d89 100644 --- a/apps/emqx_authn/src/emqx_authn_schema.erl +++ b/apps/emqx_authn/src/emqx_authn_schema.erl @@ -105,13 +105,10 @@ select_union_member(Value, _Providers) -> throw(#{reason => "not_a_struct", value => Value}). try_select_union_member(Module, Value) -> - %% some modules have refs/1 exported to help selectin the sub-types + %% some modules have union_member_selector/1 exported to help selectin the sub-types %% emqx_authn_http, emqx_authn_jwt, emqx_authn_mongodb and emqx_authn_redis - try Module:refs(Value) of - {ok, Type} -> - [Type]; - {error, Reason} -> - throw(Reason) + try + Module:union_member_selector({value, Value}) catch error:undef -> %% otherwise expect only one member from this module diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl index 0c0ec3b06..9be1d0a33 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl @@ -40,7 +40,7 @@ -export([ refs/0, - refs/1, + union_member_selector/1, create/2, update/2, authenticate/2, @@ -60,7 +60,7 @@ roots() -> [ {?CONF_NS, hoconsc:mk( - hoconsc:union(refs()), + hoconsc:union(fun union_member_selector/1), #{} )} ]. @@ -160,15 +160,20 @@ refs() -> hoconsc:ref(?MODULE, post) ]. +union_member_selector(all_union_members) -> + refs(); +union_member_selector({value, Value}) -> + refs(Value). + refs(#{<<"method">> := <<"get">>}) -> - {ok, hoconsc:ref(?MODULE, get)}; + [hoconsc:ref(?MODULE, get)]; refs(#{<<"method">> := <<"post">>}) -> - {ok, hoconsc:ref(?MODULE, post)}; + [hoconsc:ref(?MODULE, post)]; refs(_) -> - {error, #{ + throw(#{ field_name => method, expected => "get | post" - }}. + }). create(_AuthenticatorID, Config) -> create(Config). diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl index d96f9073e..4de64dcfe 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_jwt.erl @@ -32,7 +32,7 @@ -export([ refs/0, - refs/1, + union_member_selector/1, create/2, update/2, authenticate/2, @@ -52,7 +52,7 @@ roots() -> [ {?CONF_NS, hoconsc:mk( - hoconsc:union(refs()), + hoconsc:union(fun union_member_selector/1), #{} )} ]. @@ -169,7 +169,9 @@ refs() -> hoconsc:ref(?MODULE, 'jwks') ]. -refs(#{<<"mechanism">> := <<"jwt">>} = V) -> +union_member_selector(all_union_members) -> + refs(); +union_member_selector({value, V}) -> UseJWKS = maps:get(<<"use_jwks">>, V, undefined), select_ref(boolean(UseJWKS), V). @@ -181,16 +183,16 @@ boolean(<<"false">>) -> false; boolean(Other) -> Other. select_ref(true, _) -> - {ok, hoconsc:ref(?MODULE, 'jwks')}; + [hoconsc:ref(?MODULE, 'jwks')]; select_ref(false, #{<<"public_key">> := _}) -> - {ok, hoconsc:ref(?MODULE, 'public-key')}; + [hoconsc:ref(?MODULE, 'public-key')]; select_ref(false, _) -> - {ok, hoconsc:ref(?MODULE, 'hmac-based')}; + [hoconsc:ref(?MODULE, 'hmac-based')]; select_ref(_, _) -> - {error, #{ + throw(#{ field_name => use_jwks, expected => "true | false" - }}. + }). create(_AuthenticatorID, Config) -> create(Config). diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl index 11048528c..22b930485 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl @@ -33,7 +33,7 @@ -export([ refs/0, - refs/1, + union_member_selector/1, create/2, update/2, authenticate/2, @@ -53,7 +53,7 @@ roots() -> [ {?CONF_NS, hoconsc:mk( - hoconsc:union(refs()), + hoconsc:union(fun union_member_selector/1), #{} )} ]. @@ -131,18 +131,6 @@ refs() -> hoconsc:ref(?MODULE, 'sharded-cluster') ]. -refs(#{<<"mongo_type">> := <<"single">>}) -> - {ok, hoconsc:ref(?MODULE, standalone)}; -refs(#{<<"mongo_type">> := <<"rs">>}) -> - {ok, hoconsc:ref(?MODULE, 'replica-set')}; -refs(#{<<"mongo_type">> := <<"sharded">>}) -> - {ok, hoconsc:ref(?MODULE, 'sharded-cluster')}; -refs(_) -> - {error, #{ - field_name => mongo_type, - expected => "single | rs | sharded" - }}. - create(_AuthenticatorID, Config) -> create(Config). @@ -259,3 +247,20 @@ is_superuser(Doc, #{is_superuser_field := IsSuperuserField}) -> emqx_authn_utils:is_superuser(#{<<"is_superuser">> => IsSuperuser}); is_superuser(_, _) -> emqx_authn_utils:is_superuser(#{<<"is_superuser">> => false}). + +union_member_selector(all_union_members) -> + refs(); +union_member_selector({value, Value}) -> + refs(Value). + +refs(#{<<"mongo_type">> := <<"single">>}) -> + [hoconsc:ref(?MODULE, standalone)]; +refs(#{<<"mongo_type">> := <<"rs">>}) -> + [hoconsc:ref(?MODULE, 'replica-set')]; +refs(#{<<"mongo_type">> := <<"sharded">>}) -> + [hoconsc:ref(?MODULE, 'sharded-cluster')]; +refs(_) -> + throw(#{ + field_name => mongo_type, + expected => "single | rs | sharded" + }). diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl index 419899bbd..ff81fd4ca 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl @@ -33,7 +33,7 @@ -export([ refs/0, - refs/1, + union_member_selector/1, create/2, update/2, authenticate/2, @@ -53,7 +53,7 @@ roots() -> [ {?CONF_NS, hoconsc:mk( - hoconsc:union(refs()), + hoconsc:union(fun union_member_selector/1), #{} )} ]. @@ -98,17 +98,22 @@ refs() -> hoconsc:ref(?MODULE, sentinel) ]. +union_member_selector(all_union_members) -> + refs(); +union_member_selector({value, Value}) -> + refs(Value). + refs(#{<<"redis_type">> := <<"single">>}) -> - {ok, hoconsc:ref(?MODULE, standalone)}; + [hoconsc:ref(?MODULE, standalone)]; refs(#{<<"redis_type">> := <<"cluster">>}) -> - {ok, hoconsc:ref(?MODULE, cluster)}; + [hoconsc:ref(?MODULE, cluster)]; refs(#{<<"redis_type">> := <<"sentinel">>}) -> - {ok, hoconsc:ref(?MODULE, sentinel)}; + [hoconsc:ref(?MODULE, sentinel)]; refs(_) -> - {error, #{ + throw(#{ field_name => redis_type, expected => "single | cluster | sentinel" - }}. + }). create(_AuthenticatorID, Config) -> create(Config). diff --git a/apps/emqx_authn/test/emqx_authn_schema_tests.erl b/apps/emqx_authn/test/emqx_authn_schema_tests.erl new file mode 100644 index 000000000..25fcd28e4 --- /dev/null +++ b/apps/emqx_authn/test/emqx_authn_schema_tests.erl @@ -0,0 +1,135 @@ +%%-------------------------------------------------------------------- +%% Copyright (c) 2023 EMQ Technologies Co., Ltd. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%%-------------------------------------------------------------------- + +-module(emqx_authn_schema_tests). + +-include_lib("eunit/include/eunit.hrl"). + +%% schema error +-define(ERR(Reason), {error, Reason}). + +union_member_selector_mongo_test_() -> + Check = fun(Txt) -> check(emqx_authn_mongodb, Txt) end, + [ + {"unknown", fun() -> + ?assertMatch( + ?ERR(#{field_name := mongo_type, expected := _}), + Check("{mongo_type: foobar}") + ) + end}, + {"single", fun() -> + ?assertMatch( + ?ERR(#{matched_type := "authn-mongodb:standalone"}), + Check("{mongo_type: single}") + ) + end}, + {"replica-set", fun() -> + ?assertMatch( + ?ERR(#{matched_type := "authn-mongodb:replica-set"}), + Check("{mongo_type: rs}") + ) + end}, + {"sharded", fun() -> + ?assertMatch( + ?ERR(#{matched_type := "authn-mongodb:sharded-cluster"}), + Check("{mongo_type: sharded}") + ) + end} + ]. + +union_member_selector_jwt_test_() -> + Check = fun(Txt) -> check(emqx_authn_jwt, Txt) end, + [ + {"unknown", fun() -> + ?assertMatch( + ?ERR(#{field_name := use_jwks, expected := "true | false"}), + Check("{use_jwks = 1}") + ) + end}, + {"jwks", fun() -> + ?assertMatch( + ?ERR(#{matched_type := "authn-jwt:jwks"}), + Check("{use_jwks = true}") + ) + end}, + {"publick-key", fun() -> + ?assertMatch( + ?ERR(#{matched_type := "authn-jwt:public-key"}), + Check("{use_jwks = false, public_key = 1}") + ) + end}, + {"hmac-based", fun() -> + ?assertMatch( + ?ERR(#{matched_type := "authn-jwt:hmac-based"}), + Check("{use_jwks = false}") + ) + end} + ]. + +union_member_selector_redis_test_() -> + Check = fun(Txt) -> check(emqx_authn_redis, Txt) end, + [ + {"unknown", fun() -> + ?assertMatch( + ?ERR(#{field_name := redis_type, expected := _}), + Check("{redis_type = 1}") + ) + end}, + {"single", fun() -> + ?assertMatch( + ?ERR(#{matched_type := "authn-redis:standalone"}), + Check("{redis_type = single}") + ) + end}, + {"cluster", fun() -> + ?assertMatch( + ?ERR(#{matched_type := "authn-redis:cluster"}), + Check("{redis_type = cluster}") + ) + end}, + {"sentinel", fun() -> + ?assertMatch( + ?ERR(#{matched_type := "authn-redis:sentinel"}), + Check("{redis_type = sentinel}") + ) + end} + ]. + +union_member_selector_http_test_() -> + Check = fun(Txt) -> check(emqx_authn_http, Txt) end, + [ + {"unknown", fun() -> + ?assertMatch( + ?ERR(#{field_name := method, expected := _}), + Check("{method = 1}") + ) + end}, + {"get", fun() -> + ?assertMatch( + ?ERR(#{matched_type := "authn-http:get"}), + Check("{method = get}") + ) + end}, + {"post", fun() -> + ?assertMatch( + ?ERR(#{matched_type := "authn-http:post"}), + Check("{method = post}") + ) + end} + ]. + +check(Module, HoconConf) -> + emqx_hocon:check(Module, ["authentication= ", HoconConf]). From 83e66da2aa90c2d2295344fa14d549a3b4f9a1b0 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Wed, 18 Jan 2023 12:40:57 +0100 Subject: [PATCH 09/11] fix(authn): throw exception on invalid config from the API --- apps/emqx/src/emqx_config.erl | 26 ++------------------- apps/emqx/src/emqx_hocon.erl | 10 ++++++--- apps/emqx_authn/src/emqx_authn.app.src | 2 +- apps/emqx_authn/src/emqx_authn.erl | 15 ++++++++----- apps/emqx_authn/src/emqx_authn_app.erl | 31 +++++++++----------------- apps/emqx_conf/src/emqx_conf.app.src | 2 +- 6 files changed, 32 insertions(+), 54 deletions(-) diff --git a/apps/emqx/src/emqx_config.erl b/apps/emqx/src/emqx_config.erl index fb84d64a1..b76234e85 100644 --- a/apps/emqx/src/emqx_config.erl +++ b/apps/emqx/src/emqx_config.erl @@ -414,32 +414,10 @@ check_config(SchemaMod, RawConf, Opts0) -> do_check_config(SchemaMod, RawConf, Opts0) catch throw:Errors:Stacktrace -> - throw(emqx_hocon:compact_errors(Errors, Stacktrace)) + {error, Reason} = emqx_hocon:compact_errors(Errors, Stacktrace), + erlang:raise(throw, Reason, Stacktrace) end. -%% HOCON tries to be very informative about all the detailed errors -%% it's maybe too much when reporting to the user --spec compact_errors(any(), any()) -> no_return(). -compact_errors(Schema, [Error0 | More]) when is_map(Error0) -> - Error1 = - case length(More) of - 0 -> - Error0; - N -> - Error0#{unshown_errors => N} - end, - Error = - case is_atom(Schema) of - true -> - Error1#{schema_module => Schema}; - false -> - Error1 - end, - throw(Error); -compact_errors(Schema, Errors) -> - %% unexpected, we need the stacktrace reported, hence error - error({Schema, Errors}). - do_check_config(SchemaMod, RawConf, Opts0) -> Opts1 = #{ return_plain => true, diff --git a/apps/emqx/src/emqx_hocon.erl b/apps/emqx/src/emqx_hocon.erl index a039aa8c2..09503e6fe 100644 --- a/apps/emqx/src/emqx_hocon.erl +++ b/apps/emqx/src/emqx_hocon.erl @@ -20,6 +20,7 @@ -export([ format_path/1, check/2, + check/3, compact_errors/2, format_error/1, format_error/2, @@ -37,20 +38,23 @@ format_path([Name | Rest]) -> [iol(Name), "." | format_path(Rest)]. %% Always return plain map with atom keys. -spec check(module(), hocon:config() | iodata()) -> {ok, hocon:config()} | {error, any()}. -check(SchemaModule, Conf) when is_map(Conf) -> +check(SchemaModule, Conf) -> %% TODO: remove required %% fields should state required or not in their schema Opts = #{atom_key => true, required => false}, + check(SchemaModule, Conf, Opts). + +check(SchemaModule, Conf, Opts) when is_map(Conf) -> try {ok, hocon_tconf:check_plain(SchemaModule, Conf, Opts)} catch throw:Errors:Stacktrace -> compact_errors(Errors, Stacktrace) end; -check(SchemaModule, HoconText) -> +check(SchemaModule, HoconText, Opts) -> case hocon:binary(HoconText, #{format => map}) of {ok, MapConfig} -> - check(SchemaModule, MapConfig); + check(SchemaModule, MapConfig, Opts); {error, Reason} -> {error, Reason} end. diff --git a/apps/emqx_authn/src/emqx_authn.app.src b/apps/emqx_authn/src/emqx_authn.app.src index 7f01d94c0..0b5b0dedc 100644 --- a/apps/emqx_authn/src/emqx_authn.app.src +++ b/apps/emqx_authn/src/emqx_authn.app.src @@ -1,7 +1,7 @@ %% -*- mode: erlang -*- {application, emqx_authn, [ {description, "EMQX Authentication"}, - {vsn, "0.1.12"}, + {vsn, "0.1.13"}, {modules, []}, {registered, [emqx_authn_sup, emqx_authn_registry]}, {applications, [kernel, stdlib, emqx_resource, emqx_connector, ehttpc, epgsql, mysql, jose]}, diff --git a/apps/emqx_authn/src/emqx_authn.erl b/apps/emqx_authn/src/emqx_authn.erl index 934c57fb1..8bda0c3a1 100644 --- a/apps/emqx_authn/src/emqx_authn.erl +++ b/apps/emqx_authn/src/emqx_authn.erl @@ -69,11 +69,7 @@ do_check_config(#{<<"mechanism">> := Mec0} = Config, Opts) -> false -> throw(#{error => unknown_authn_provider, which => Key}); {_, ProviderModule} -> - emqx_hocon:check( - ProviderModule, - #{?CONF_NS_BINARY => Config}, - Opts#{atom_key => true} - ) + do_check_config_maybe_throw(ProviderModule, Config, Opts) end; do_check_config(Config, Opts) when is_map(Config) -> throw(#{ @@ -82,6 +78,15 @@ do_check_config(Config, Opts) when is_map(Config) -> reason => "mechanism_field_required" }). +do_check_config_maybe_throw(ProviderModule, Config0, Opts) -> + Config = #{?CONF_NS_BINARY => Config0}, + case emqx_hocon:check(ProviderModule, Config, Opts#{atom_key => true}) of + {ok, Checked} -> + Checked; + {error, Reason} -> + throw(Reason) + end. + %% The atoms have to be loaded already, %% which might be an issue for plugins which are loaded after node boot %% but they should really manage their own configs in that case. diff --git a/apps/emqx_authn/src/emqx_authn_app.erl b/apps/emqx_authn/src/emqx_authn_app.erl index ed97c0975..c94aec75d 100644 --- a/apps/emqx_authn/src/emqx_authn_app.erl +++ b/apps/emqx_authn/src/emqx_authn_app.erl @@ -53,26 +53,17 @@ stop(_State) -> %%------------------------------------------------------------------------------ initialize() -> - try - ok = ?AUTHN:register_providers(emqx_authn:providers()), - - lists:foreach( - fun({ChainName, RawAuthConfigs}) -> - AuthConfig = emqx_authn:check_configs(RawAuthConfigs), - ?AUTHN:initialize_authentication( - ChainName, - AuthConfig - ) - end, - chain_configs() - ) - of - ok -> ok - catch - throw:Reason -> - ?SLOG(error, #{msg => "failed_to_initialize_authentication", reason => Reason}), - {error, {failed_to_initialize_authentication, Reason}} - end. + ok = ?AUTHN:register_providers(emqx_authn:providers()), + lists:foreach( + fun({ChainName, RawAuthConfigs}) -> + AuthConfig = emqx_authn:check_configs(RawAuthConfigs), + ?AUTHN:initialize_authentication( + ChainName, + AuthConfig + ) + end, + chain_configs() + ). deinitialize() -> ok = ?AUTHN:deregister_providers(provider_types()), diff --git a/apps/emqx_conf/src/emqx_conf.app.src b/apps/emqx_conf/src/emqx_conf.app.src index f7fd33e3b..beac051a1 100644 --- a/apps/emqx_conf/src/emqx_conf.app.src +++ b/apps/emqx_conf/src/emqx_conf.app.src @@ -1,6 +1,6 @@ {application, emqx_conf, [ {description, "EMQX configuration management"}, - {vsn, "0.1.11"}, + {vsn, "0.1.12"}, {registered, []}, {mod, {emqx_conf_app, []}}, {applications, [kernel, stdlib]}, From dc8f0c5101f867bc81eadad0b81490b4df62392c Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Mon, 6 Feb 2023 18:15:01 +0100 Subject: [PATCH 10/11] refactor(authn): no need to check config at app boot --- apps/emqx/src/emqx_hocon.erl | 4 +-- apps/emqx_authn/src/emqx_authn.erl | 23 +++++++--------- apps/emqx_authn/src/emqx_authn_app.erl | 7 +++-- apps/emqx_authn/test/emqx_authn_SUITE.erl | 32 +++++++++++++++++++++++ 4 files changed, 46 insertions(+), 20 deletions(-) diff --git a/apps/emqx/src/emqx_hocon.erl b/apps/emqx/src/emqx_hocon.erl index 09503e6fe..08192e9be 100644 --- a/apps/emqx/src/emqx_hocon.erl +++ b/apps/emqx/src/emqx_hocon.erl @@ -119,8 +119,8 @@ compact_errors(SchemaModule, [Error0 | More], _Stacktrace) when is_map(Error0) - end, {error, Error}; compact_errors(SchemaModule, Error, Stacktrace) -> - %% unexpected, we need the stacktrace reported, hence error - %% if this happens i'ts a bug in hocon_tconf + %% unexpected, we need the stacktrace reported + %% if this happens it's a bug in hocon_tconf {error, #{ schema_module => SchemaModule, exception => Error, diff --git a/apps/emqx_authn/src/emqx_authn.erl b/apps/emqx_authn/src/emqx_authn.erl index 8bda0c3a1..15efeb673 100644 --- a/apps/emqx_authn/src/emqx_authn.erl +++ b/apps/emqx_authn/src/emqx_authn.erl @@ -20,7 +20,6 @@ providers/0, check_config/1, check_config/2, - check_configs/1, %% for telemetry information get_enabled_authns/0 ]). @@ -39,16 +38,6 @@ providers() -> {{scram, built_in_database}, emqx_enhanced_authn_scram_mnesia} ]. -check_configs(CM) when is_map(CM) -> - check_configs([CM]); -check_configs(CL) -> - check_configs(CL, 1). - -check_configs([], _Nth) -> - []; -check_configs([Config | Configs], Nth) -> - [check_config(Config, #{id_for_log => Nth}) | check_configs(Configs, Nth + 1)]. - check_config(Config) -> check_config(Config, #{}). @@ -67,14 +56,20 @@ do_check_config(#{<<"mechanism">> := Mec0} = Config, Opts) -> end, case lists:keyfind(Key, 1, providers()) of false -> - throw(#{error => unknown_authn_provider, which => Key}); + Reason = + case Key of + {M, B} -> + #{mechanism => M, backend => B}; + M -> + #{mechanism => M} + end, + throw(Reason#{error => unknown_authn_provider}); {_, ProviderModule} -> do_check_config_maybe_throw(ProviderModule, Config, Opts) end; -do_check_config(Config, Opts) when is_map(Config) -> +do_check_config(Config, _Opts) when is_map(Config) -> throw(#{ error => invalid_config, - which => maps:get(id_for_log, Opts, unknown), reason => "mechanism_field_required" }). diff --git a/apps/emqx_authn/src/emqx_authn_app.erl b/apps/emqx_authn/src/emqx_authn_app.erl index c94aec75d..44fec2363 100644 --- a/apps/emqx_authn/src/emqx_authn_app.erl +++ b/apps/emqx_authn/src/emqx_authn_app.erl @@ -55,8 +55,7 @@ stop(_State) -> initialize() -> ok = ?AUTHN:register_providers(emqx_authn:providers()), lists:foreach( - fun({ChainName, RawAuthConfigs}) -> - AuthConfig = emqx_authn:check_configs(RawAuthConfigs), + fun({ChainName, AuthConfig}) -> ?AUTHN:initialize_authentication( ChainName, AuthConfig @@ -73,12 +72,12 @@ chain_configs() -> [global_chain_config() | listener_chain_configs()]. global_chain_config() -> - {?GLOBAL, emqx:get_raw_config([?EMQX_AUTHENTICATION_CONFIG_ROOT_NAME_BINARY], [])}. + {?GLOBAL, emqx:get_config([?EMQX_AUTHENTICATION_CONFIG_ROOT_NAME_BINARY], [])}. listener_chain_configs() -> lists:map( fun({ListenerID, _}) -> - {ListenerID, emqx:get_raw_config(auth_config_path(ListenerID), [])} + {ListenerID, emqx:get_config(auth_config_path(ListenerID), [])} end, emqx_listeners:list() ). diff --git a/apps/emqx_authn/test/emqx_authn_SUITE.erl b/apps/emqx_authn/test/emqx_authn_SUITE.erl index b30d2fcff..4f96ca2dd 100644 --- a/apps/emqx_authn/test/emqx_authn_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_SUITE.erl @@ -168,6 +168,38 @@ t_password_undefined(Config) when is_list(Config) -> end, ok. +t_union_selector_errors({init, Config}) -> + Config; +t_union_selector_errors({'end', _Config}) -> + ok; +t_union_selector_errors(Config) when is_list(Config) -> + Conf0 = #{ + <<"mechanism">> => <<"password_based">>, + <<"backend">> => <<"mysql">> + }, + Conf1 = Conf0#{<<"mechanism">> => <<"unknown-atom-xx">>}, + ?assertThrow(#{error := unknown_mechanism}, emqx_authn:check_config(Conf1)), + Conf2 = Conf0#{<<"backend">> => <<"unknown-atom-xx">>}, + ?assertThrow(#{error := unknown_backend}, emqx_authn:check_config(Conf2)), + Conf3 = Conf0#{<<"backend">> => <<"unknown">>, <<"mechanism">> => <<"unknown">>}, + ?assertThrow( + #{ + error := unknown_authn_provider, + backend := unknown, + mechanism := unknown + }, + emqx_authn:check_config(Conf3) + ), + Res = catch (emqx_authn:check_config(#{<<"mechanism">> => <<"unknown">>})), + ?assertEqual( + #{ + error => unknown_authn_provider, + mechanism => unknown + }, + Res + ), + ok. + parse(Bytes) -> {ok, Frame, <<>>, {none, _}} = emqx_frame:parse(Bytes), Frame. From 537ff305505c367b92b60aa9c434bdce5a17b880 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Tue, 7 Feb 2023 11:40:17 +0100 Subject: [PATCH 11/11] chore: update code comment --- apps/emqx_authn/src/emqx_authn_schema.erl | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/apps/emqx_authn/src/emqx_authn_schema.erl b/apps/emqx_authn/src/emqx_authn_schema.erl index f4a966d89..112ea2076 100644 --- a/apps/emqx_authn/src/emqx_authn_schema.erl +++ b/apps/emqx_authn/src/emqx_authn_schema.erl @@ -105,8 +105,12 @@ select_union_member(Value, _Providers) -> throw(#{reason => "not_a_struct", value => Value}). try_select_union_member(Module, Value) -> - %% some modules have union_member_selector/1 exported to help selectin the sub-types - %% emqx_authn_http, emqx_authn_jwt, emqx_authn_mongodb and emqx_authn_redis + %% some modules have `union_member_selector/1' exported to help selecting + %% the sub-types, they are: + %% emqx_authn_http + %% emqx_authn_jwt + %% emqx_authn_mongodb + %% emqx_authn_redis try Module:union_member_selector({value, Value}) catch