diff --git a/.github/workflows/run_fvt_tests.yaml b/.github/workflows/run_fvt_tests.yaml index 0cf530765..6a8c3f186 100644 --- a/.github/workflows/run_fvt_tests.yaml +++ b/.github/workflows/run_fvt_tests.yaml @@ -141,7 +141,9 @@ jobs: path: . - name: unzip source code run: unzip -q source.zip - + - uses: erlef/setup-beam@v1 + with: + otp-version: "24.2" - name: Get deps git refs for cache id: deps-refs run: | diff --git a/apps/emqx/src/emqx_authentication.erl b/apps/emqx/src/emqx_authentication.erl index 33d2c20cd..b866c35f9 100644 --- a/apps/emqx/src/emqx_authentication.erl +++ b/apps/emqx/src/emqx_authentication.erl @@ -47,7 +47,6 @@ register_providers/1, deregister_provider/1, deregister_providers/1, - create_chain/1, delete_chain/1, lookup_chain/1, list_chains/0, @@ -271,7 +270,6 @@ authenticator_id(Config) -> initialize_authentication(_, []) -> ok; initialize_authentication(ChainName, AuthenticatorsConfig) -> - _ = create_chain(ChainName), CheckedConfig = to_list(AuthenticatorsConfig), lists:foreach( fun(AuthenticatorConfig) -> @@ -322,10 +320,6 @@ deregister_providers(AuthNTypes) when is_list(AuthNTypes) -> deregister_provider(AuthNType) -> deregister_providers([AuthNType]). --spec create_chain(chain_name()) -> {ok, chain()} | {error, term()}. -create_chain(Name) -> - call({create_chain, Name}). - -spec delete_chain(chain_name()) -> ok | {error, term()}. delete_chain(Name) -> call({delete_chain, Name}). @@ -451,50 +445,36 @@ handle_call( end; handle_call({deregister_providers, AuthNTypes}, _From, #{providers := Providers} = State) -> reply(ok, State#{providers := maps:without(AuthNTypes, Providers)}); -handle_call({create_chain, Name}, _From, State) -> - case ets:member(?CHAINS_TAB, Name) of - true -> - reply({error, {already_exists, {chain, Name}}}, State); - false -> - Chain = #chain{ - name = Name, - authenticators = [] - }, - true = ets:insert(?CHAINS_TAB, Chain), - reply({ok, serialize_chain(Chain)}, State) - end; -handle_call({delete_chain, Name}, _From, State) -> - case ets:lookup(?CHAINS_TAB, Name) of - [] -> - reply({error, {not_found, {chain, Name}}}, State); - [#chain{} = Chain] -> - _MatchedIDs = do_delete_authenticators(fun(_) -> true end, Chain), - true = ets:delete(?CHAINS_TAB, Name), - reply(ok, maybe_unhook(State)) - end; +handle_call({delete_chain, ChainName}, _From, State) -> + UpdateFun = fun(Chain) -> + {_MatchedIDs, NewChain} = do_delete_authenticators(fun(_) -> true end, Chain), + {ok, ok, NewChain} + end, + Reply = with_chain(ChainName, UpdateFun), + reply(Reply, maybe_unhook(State)); handle_call({create_authenticator, ChainName, Config}, _From, #{providers := Providers} = State) -> UpdateFun = fun(Chain) -> handle_create_authenticator(Chain, Config, Providers) end, - Reply = update_chain(ChainName, UpdateFun), + Reply = with_new_chain(ChainName, UpdateFun), reply(Reply, maybe_hook(State)); handle_call({delete_authenticator, ChainName, AuthenticatorID}, _From, State) -> UpdateFun = fun(Chain) -> handle_delete_authenticator(Chain, AuthenticatorID) end, - Reply = update_chain(ChainName, UpdateFun), + Reply = with_chain(ChainName, UpdateFun), reply(Reply, maybe_unhook(State)); handle_call({update_authenticator, ChainName, AuthenticatorID, Config}, _From, State) -> UpdateFun = fun(Chain) -> handle_update_authenticator(Chain, AuthenticatorID, Config) end, - Reply = update_chain(ChainName, UpdateFun), + Reply = with_chain(ChainName, UpdateFun), reply(Reply, State); handle_call({move_authenticator, ChainName, AuthenticatorID, Position}, _From, State) -> UpdateFun = fun(Chain) -> handle_move_authenticator(Chain, AuthenticatorID, Position) end, - Reply = update_chain(ChainName, UpdateFun), + Reply = with_chain(ChainName, UpdateFun), reply(Reply, State); handle_call({import_users, ChainName, AuthenticatorID, Filename}, _From, State) -> Reply = call_authenticator(ChainName, AuthenticatorID, import_users, [Filename]), @@ -569,11 +549,9 @@ handle_update_authenticator(Chain, AuthenticatorID, Config) -> NewAuthenticator, Authenticators ), - true = ets:insert( - ?CHAINS_TAB, - Chain#chain{authenticators = NewAuthenticators} - ), - {ok, serialize_authenticator(NewAuthenticator)}; + NewChain = Chain#chain{authenticators = NewAuthenticators}, + Result = {ok, serialize_authenticator(NewAuthenticator)}, + {ok, Result, NewChain}; {error, Reason} -> {error, Reason} end; @@ -587,18 +565,18 @@ handle_delete_authenticator(Chain, AuthenticatorID) -> ID =:= AuthenticatorID end, case do_delete_authenticators(MatchFun, Chain) of - [] -> + {[], _NewChain} -> {error, {not_found, {authenticator, AuthenticatorID}}}; - [AuthenticatorID] -> - ok + {[AuthenticatorID], NewChain} -> + {ok, ok, NewChain} end. handle_move_authenticator(Chain, AuthenticatorID, Position) -> #chain{authenticators = Authenticators} = Chain, case do_move_authenticator(AuthenticatorID, Authenticators, Position) of {ok, NAuthenticators} -> - true = ets:insert(?CHAINS_TAB, Chain#chain{authenticators = NAuthenticators}), - ok; + NewChain = Chain#chain{authenticators = NAuthenticators}, + {ok, ok, NewChain}; {error, Reason} -> {error, Reason} end. @@ -616,18 +594,15 @@ handle_create_authenticator(Chain, Config, Providers) -> NAuthenticators = Authenticators ++ [Authenticator#authenticator{enable = maps:get(enable, Config)}], - true = ets:insert( - ?CHAINS_TAB, - Chain#chain{authenticators = NAuthenticators} - ), - ok = emqx_metrics_worker:create_metrics( authn_metrics, metrics_id(Name, AuthenticatorID), [total, success, failed, nomatch], [total] ), - {ok, serialize_authenticator(Authenticator)}; + NewChain = Chain#chain{authenticators = NAuthenticators}, + Result = {ok, serialize_authenticator(Authenticator)}, + {ok, Result, NewChain}; {error, Reason} -> {error, Reason} end @@ -675,6 +650,14 @@ do_authenticate( reply(Reply, State) -> {reply, Reply, State}. +save_chain(#chain{ + name = Name, + authenticators = [] +}) -> + ets:delete(?CHAINS_TAB, Name); +save_chain(#chain{} = Chain) -> + ets:insert(?CHAINS_TAB, Chain). + create_chain_table() -> try _ = ets:new(?CHAINS_TAB, [ @@ -774,14 +757,7 @@ do_delete_authenticators(MatchFun, #chain{name = Name, authenticators = Authenti end, Matching ), - true = - case Others of - [] -> - ets:delete(?CHAINS_TAB, Name); - _ -> - ets:insert(?CHAINS_TAB, Chain#chain{authenticators = Others}) - end, - MatchingIDs. + {MatchingIDs, Chain#chain{authenticators = Others}}. do_destroy_authenticator(#authenticator{provider = Provider, state = State}) -> _ = Provider:destroy(State), @@ -824,21 +800,41 @@ insert( insert(Authenticator, [Authenticator0 | More], {Relative, RelatedID}, Acc) -> insert(Authenticator, More, {Relative, RelatedID}, [Authenticator0 | Acc]). -update_chain(ChainName, UpdateFun) -> +with_new_chain(ChainName, Fun) -> + case ets:lookup(?CHAINS_TAB, ChainName) of + [] -> + Chain = #chain{name = ChainName, authenticators = []}, + do_with_chain(Fun, Chain); + [Chain] -> + do_with_chain(Fun, Chain) + end. + +with_chain(ChainName, Fun) -> case ets:lookup(?CHAINS_TAB, ChainName) of [] -> {error, {not_found, {chain, ChainName}}}; [Chain] -> - try - UpdateFun(Chain) - catch - Class:Reason:Stk -> - {error, {exception, {Class, Reason, Stk}}} - end + do_with_chain(Fun, Chain) + end. + +do_with_chain(Fun, Chain) -> + try + case Fun(Chain) of + {ok, Result} -> + Result; + {ok, Result, NewChain} -> + save_chain(NewChain), + Result; + {error, _} = Error -> + Error + end + catch + Class:Reason:Stk -> + {error, {exception, {Class, Reason, Stk}}} end. call_authenticator(ChainName, AuthenticatorID, Func, Args) -> - UpdateFun = + Fun = fun(#chain{authenticators = Authenticators}) -> case lists:keyfind(AuthenticatorID, #authenticator.id, Authenticators) of false -> @@ -846,13 +842,13 @@ call_authenticator(ChainName, AuthenticatorID, Func, Args) -> #authenticator{provider = Provider, state = State} -> case erlang:function_exported(Provider, Func, length(Args) + 1) of true -> - erlang:apply(Provider, Func, Args ++ [State]); + {ok, erlang:apply(Provider, Func, Args ++ [State])}; false -> {error, unsupported_operation} end end end, - update_chain(ChainName, UpdateFun). + with_chain(ChainName, Fun). serialize_chain(#chain{ name = Name, diff --git a/apps/emqx/src/emqx_authentication_config.erl b/apps/emqx/src/emqx_authentication_config.erl index 93fcafc35..681ed1394 100644 --- a/apps/emqx/src/emqx_authentication_config.erl +++ b/apps/emqx/src/emqx_authentication_config.erl @@ -140,7 +140,6 @@ post_config_update(_, UpdateReq, NewConfig, OldConfig, AppEnvs) -> do_post_config_update({create_authenticator, ChainName, Config}, NewConfig, _OldConfig, _AppEnvs) -> NConfig = get_authenticator_config(authenticator_id(Config), NewConfig), - _ = emqx_authentication:create_chain(ChainName), emqx_authentication:create_authenticator(ChainName, NConfig); do_post_config_update( {delete_authenticator, ChainName, AuthenticatorID}, diff --git a/apps/emqx/test/emqx_authentication_SUITE.erl b/apps/emqx/test/emqx_authentication_SUITE.erl index 63aafb124..61b4b2775 100644 --- a/apps/emqx/test/emqx_authentication_SUITE.erl +++ b/apps/emqx/test/emqx_authentication_SUITE.erl @@ -113,20 +113,32 @@ end_per_testcase(Case, Config) -> _ = ?MODULE:Case({'end', Config}), ok. -t_chain({_, Config}) -> +t_chain({'init', Config}) -> Config; t_chain(Config) when is_list(Config) -> % CRUD of authentication chain ChainName = 'test', ?assertMatch({ok, []}, ?AUTHN:list_chains()), ?assertMatch({ok, []}, ?AUTHN:list_chain_names()), - ?assertMatch({ok, #{name := ChainName, authenticators := []}}, ?AUTHN:create_chain(ChainName)), - ?assertEqual({error, {already_exists, {chain, ChainName}}}, ?AUTHN:create_chain(ChainName)), - ?assertMatch({ok, #{name := ChainName, authenticators := []}}, ?AUTHN:lookup_chain(ChainName)), + + %% to create a chain we need create an authenticator + AuthenticatorConfig = #{ + mechanism => password_based, + backend => built_in_database, + enable => true + }, + register_provider({password_based, built_in_database}, ?MODULE), + ?AUTHN:create_authenticator(ChainName, AuthenticatorConfig), + + ?assertMatch({ok, #{name := ChainName, authenticators := [_]}}, ?AUTHN:lookup_chain(ChainName)), ?assertMatch({ok, [#{name := ChainName}]}, ?AUTHN:list_chains()), ?assertEqual({ok, [ChainName]}, ?AUTHN:list_chain_names()), ?assertEqual(ok, ?AUTHN:delete_chain(ChainName)), ?assertMatch({error, {not_found, {chain, ChainName}}}, ?AUTHN:lookup_chain(ChainName)), + ok; +t_chain({'end', _Config}) -> + ?AUTHN:delete_chain(test), + ?AUTHN:deregister_providers([{password_based, built_in_database}]), ok. t_authenticator({'init', Config}) -> @@ -143,13 +155,6 @@ t_authenticator(Config) when is_list(Config) -> enable => true }, - % Create an authenticator when the authentication chain does not exist - ?assertEqual( - {error, {not_found, {chain, ChainName}}}, - ?AUTHN:create_authenticator(ChainName, AuthenticatorConfig1) - ), - - ?AUTHN:create_chain(ChainName), % Create an authenticator when the provider does not exist ?assertEqual( @@ -183,11 +188,14 @@ t_authenticator(Config) when is_list(Config) -> ?assertEqual(ok, ?AUTHN:delete_authenticator(ChainName, ID1)), ?assertEqual( - {error, {not_found, {authenticator, ID1}}}, + {error, {not_found, {chain, test}}}, ?AUTHN:update_authenticator(ChainName, ID1, AuthenticatorConfig1) ), - ?assertMatch({ok, []}, ?AUTHN:list_authenticators(ChainName)), + ?assertMatch( + {error, {not_found, {chain, ChainName}}}, + ?AUTHN:list_authenticators(ChainName) + ), % Multiple authenticators exist at the same time AuthNType2 = ?config("auth2"), @@ -253,7 +261,6 @@ t_authenticate(Config) when is_list(Config) -> backend => built_in_database, enable => true }, - ?AUTHN:create_chain(ListenerID), ?assertMatch({ok, _}, ?AUTHN:create_authenticator(ListenerID, AuthenticatorConfig)), ?assertEqual( @@ -352,7 +359,7 @@ t_update_config(Config) when is_list(Config) -> ), ?assertEqual( - {error, {not_found, {authenticator, ID2}}}, + {error, {not_found, {chain, Global}}}, ?AUTHN:lookup_authenticator(Global, ID2) ), @@ -427,7 +434,15 @@ t_restart({'init', Config}) -> t_restart(Config) when is_list(Config) -> ?assertEqual({ok, []}, ?AUTHN:list_chain_names()), - ?AUTHN:create_chain(test_chain), + %% to create a chain we need create an authenticator + AuthenticatorConfig = #{ + mechanism => password_based, + backend => built_in_database, + enable => true + }, + register_provider({password_based, built_in_database}, ?MODULE), + ?AUTHN:create_authenticator(test_chain, AuthenticatorConfig), + ?assertEqual({ok, [test_chain]}, ?AUTHN:list_chain_names()), ok = supervisor:terminate_child(emqx_authentication_sup, ?AUTHN), @@ -436,6 +451,7 @@ t_restart(Config) when is_list(Config) -> ?assertEqual({ok, [test_chain]}, ?AUTHN:list_chain_names()); t_restart({'end', _Config}) -> ?AUTHN:delete_chain(test_chain), + ?AUTHN:deregister_providers([{password_based, built_in_database}]), ok. t_convert_certs({_, Config}) -> diff --git a/apps/emqx_authn/test/emqx_authn_http_SUITE.erl b/apps/emqx_authn/test/emqx_authn_http_SUITE.erl index dce4522f3..a22c7765e 100644 --- a/apps/emqx_authn/test/emqx_authn_http_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_http_SUITE.erl @@ -101,7 +101,10 @@ t_create_invalid(_Config) -> throw:Error -> {error, Error} end, - {ok, []} = emqx_authentication:list_authenticators(?GLOBAL) + ?assertEqual( + {error, {not_found, {chain, ?GLOBAL}}}, + emqx_authentication:list_authenticators(?GLOBAL) + ) end, InvalidConfigs ). diff --git a/apps/emqx_authn/test/emqx_authn_mongo_SUITE.erl b/apps/emqx_authn/test/emqx_authn_mongo_SUITE.erl index 1f3667efb..dfdc0fdd1 100644 --- a/apps/emqx_authn/test/emqx_authn_mongo_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_mongo_SUITE.erl @@ -95,7 +95,10 @@ t_create_invalid(_Config) -> {create_authenticator, ?GLOBAL, Config} ), - {ok, []} = emqx_authentication:list_authenticators(?GLOBAL) + ?assertEqual( + {error, {not_found, {chain, ?GLOBAL}}}, + emqx_authentication:list_authenticators(?GLOBAL) + ) end, InvalidConfigs ). diff --git a/apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl b/apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl index fd87335b4..c8bee9223 100644 --- a/apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl @@ -113,7 +113,10 @@ t_create_invalid(_Config) -> {create_authenticator, ?GLOBAL, Config} ), emqx_authn_test_lib:delete_config(?ResourceID), - {ok, _} = emqx_authentication:list_authenticators(?GLOBAL) + ?assertEqual( + {error, {not_found, {chain, ?GLOBAL}}}, + emqx_authentication:list_authenticators(?GLOBAL) + ) end, InvalidConfigs ). diff --git a/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl b/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl index 60b789bcf..618768e42 100644 --- a/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl @@ -114,7 +114,10 @@ t_create_invalid(_Config) -> {create_authenticator, ?GLOBAL, Config} ), emqx_authn_test_lib:delete_config(?ResourceID), - {ok, []} = emqx_authentication:list_authenticators(?GLOBAL) + ?assertEqual( + {error, {not_found, {chain, ?GLOBAL}}}, + emqx_authentication:list_authenticators(?GLOBAL) + ) end, InvalidConfigs ). diff --git a/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl b/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl index 163044c21..1ff391729 100644 --- a/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl @@ -85,8 +85,10 @@ end_per_suite(_Config) -> %%------------------------------------------------------------------------------ t_create(_Config) -> - {ok, []} = emqx_authentication:list_authenticators(?GLOBAL), - + ?assertEqual( + {error, {not_found, {chain, ?GLOBAL}}}, + emqx_authentication:list_authenticators(?GLOBAL) + ), AuthConfig = raw_redis_auth_config(), {ok, _} = emqx:update_config( ?PATH, @@ -119,7 +121,10 @@ t_create_invalid(_Config) -> {create_authenticator, ?GLOBAL, Config} ), - {ok, []} = emqx_authentication:list_authenticators(?GLOBAL) + ?assertEqual( + {error, {not_found, {chain, ?GLOBAL}}}, + emqx_authentication:list_authenticators(?GLOBAL) + ) end, InvalidConfigs ), @@ -139,7 +144,10 @@ t_create_invalid(_Config) -> {create_authenticator, ?GLOBAL, Config} ), emqx_authn_test_lib:delete_config(?ResourceID), - {ok, []} = emqx_authentication:list_authenticators(?GLOBAL) + ?assertEqual( + {error, {not_found, {chain, ?GLOBAL}}}, + emqx_authentication:list_authenticators(?GLOBAL) + ) end, InvalidConfigs1 ). diff --git a/apps/emqx_authn/test/emqx_enhanced_authn_scram_mnesia_SUITE.erl b/apps/emqx_authn/test/emqx_enhanced_authn_scram_mnesia_SUITE.erl index 8e1963791..86558e5ec 100644 --- a/apps/emqx_authn/test/emqx_enhanced_authn_scram_mnesia_SUITE.erl +++ b/apps/emqx_authn/test/emqx_enhanced_authn_scram_mnesia_SUITE.erl @@ -87,7 +87,10 @@ t_create_invalid(_Config) -> {create_authenticator, ?GLOBAL, InvalidConfig} ), - {ok, []} = emqx_authentication:list_authenticators(?GLOBAL). + ?assertEqual( + {error, {not_found, {chain, ?GLOBAL}}}, + emqx_authentication:list_authenticators(?GLOBAL) + ). t_authenticate(_Config) -> Algorithm = sha512, diff --git a/apps/emqx_gateway/src/emqx_gateway_insta_sup.erl b/apps/emqx_gateway/src/emqx_gateway_insta_sup.erl index a065ea8fd..c47b8a050 100644 --- a/apps/emqx_gateway/src/emqx_gateway_insta_sup.erl +++ b/apps/emqx_gateway/src/emqx_gateway_insta_sup.erl @@ -277,39 +277,19 @@ authn_conf(Conf) -> maps:get(authentication, Conf, #{enable => false}). do_create_authn_chain(ChainName, AuthConf) -> - case ensure_chain(ChainName) of - ok -> - case emqx_authentication:create_authenticator(ChainName, AuthConf) of - {ok, _} -> - ok; - {error, Reason} -> - ?SLOG(error, #{ - msg => "failed_to_create_authenticator", - chain_name => ChainName, - reason => Reason, - config => AuthConf - }), - throw({badauth, Reason}) - end; + case emqx_authentication:create_authenticator(ChainName, AuthConf) of + {ok, _} -> + ok; {error, Reason} -> ?SLOG(error, #{ - msg => "failed_to_create_authn_chanin", + msg => "failed_to_create_authenticator", chain_name => ChainName, - reason => Reason + reason => Reason, + config => AuthConf }), throw({badauth, Reason}) end. -ensure_chain(ChainName) -> - case emqx_authentication:create_chain(ChainName) of - {ok, _ChainInfo} -> - ok; - {error, {already_exists, _}} -> - ok; - {error, Reason} -> - {error, Reason} - end. - do_deinit_authn(Names) -> lists:foreach( fun(ChainName) ->