From 096e85dc14fc1d041ae611cc87d39a959cbafd90 Mon Sep 17 00:00:00 2001 From: zhouzb Date: Sun, 26 Sep 2021 11:31:19 +0800 Subject: [PATCH] chore(authn): improve log and fix disabled authentication still working --- apps/emqx/src/emqx_authentication.erl | 53 ++++++++++++------- .../src/simple_authn/emqx_authn_http.erl | 6 +-- .../emqx_authn_jwks_connector.erl | 12 +++-- .../src/simple_authn/emqx_authn_mongodb.erl | 8 ++- .../src/simple_authn/emqx_authn_redis.erl | 4 +- 5 files changed, 56 insertions(+), 27 deletions(-) diff --git a/apps/emqx/src/emqx_authentication.erl b/apps/emqx/src/emqx_authentication.erl index 3e98fb548..341103781 100644 --- a/apps/emqx/src/emqx_authentication.erl +++ b/apps/emqx/src/emqx_authentication.erl @@ -289,16 +289,10 @@ check_config(Config) -> %%------------------------------------------------------------------------------ authenticate(#{listener := Listener, protocol := Protocol} = Credential, _AuthResult) -> - case ets:lookup(?CHAINS_TAB, Listener) of - [#chain{authenticators = Authenticators}] when Authenticators =/= [] -> - do_authenticate(Authenticators, Credential); - _ -> - case ets:lookup(?CHAINS_TAB, global_chain(Protocol)) of - [#chain{authenticators = Authenticators}] when Authenticators =/= [] -> - do_authenticate(Authenticators, Credential); - _ -> - ignore - end + Authenticators = get_authenticators(Listener, global_chain(Protocol)), + case get_enabled(Authenticators) of + [] -> ignore; + NAuthenticators -> do_authenticate(NAuthenticators, Credential) end. do_authenticate([], _) -> @@ -315,11 +309,31 @@ do_authenticate([#authenticator{id = ID, provider = Provider, state = State} | M %% {error, Reason} {stop, Result} catch - error:Reason:Stacktrace -> - ?LOG(warning, "The following error occurred in '~s' during authentication: ~p", [ID, {Reason, Stacktrace}]), + Class:Reason:Stacktrace -> + ?SLOG(warning, #{msg => "an unexpected error in authentication", + class => Class, + reason => Reason, + stacktrace => Stacktrace, + authenticator => ID}), do_authenticate(More, Credential) end. +get_authenticators(Listener, Global) -> + case ets:lookup(?CHAINS_TAB, Listener) of + [#chain{authenticators = Authenticators}] -> + Authenticators; + _ -> + case ets:lookup(?CHAINS_TAB, Global) of + [#chain{authenticators = Authenticators}] -> + Authenticators; + _ -> + [] + end + end. + +get_enabled(Authenticators) -> + [Authenticator || Authenticator <- Authenticators, Authenticator#authenticator.enable =:= true]. + %%------------------------------------------------------------------------------ %% APIs %%------------------------------------------------------------------------------ @@ -335,7 +349,9 @@ initialize_authentication(ChainName, AuthenticatorsConfig) -> {ok, _} -> ok; {error, Reason} -> - ?LOG(error, "Failed to create authenticator '~s': ~p", [generate_id(AuthenticatorConfig), Reason]) + ?SLOG(error, #{msg => "failed to create authenticator", + reason => Reason, + authenticator => generate_id(AuthenticatorConfig)}) end end, CheckedConfig). @@ -540,7 +556,7 @@ handle_call({create_authenticator, ChainName, Config}, _From, #{providers := Pro false -> case do_create_authenticator(ChainName, AuthenticatorID, Config, Providers) of {ok, Authenticator} -> - NAuthenticators = Authenticators ++ [Authenticator], + NAuthenticators = Authenticators ++ [Authenticator#authenticator{enable = maps:get(enable, Config)}], true = ets:insert(?CHAINS_TAB, Chain#chain{authenticators = NAuthenticators}), {ok, serialize_authenticator(Authenticator)}; {error, Reason} -> @@ -579,7 +595,8 @@ handle_call({update_authenticator, ChainName, AuthenticatorID, Config}, _From, S Unique = unique(ChainName, AuthenticatorID, Version), case Provider:update(Config#{'_unique' => Unique}, ST) of {ok, NewST} -> - NewAuthenticator = Authenticator#authenticator{state = switch_version(NewST)}, + NewAuthenticator = Authenticator#authenticator{state = switch_version(NewST), + enable = maps:get(enable, Config)}, NewAuthenticators = replace_authenticator(AuthenticatorID, NewAuthenticator, Authenticators), true = ets:insert(?CHAINS_TAB, Chain#chain{authenticators = NewAuthenticators}), {ok, serialize_authenticator(NewAuthenticator)}; @@ -633,15 +650,15 @@ handle_call({list_users, ChainName, AuthenticatorID}, _From, State) -> reply(Reply, State); handle_call(Req, _From, State) -> - ?LOG(error, "Unexpected call: ~p", [Req]), + ?SLOG(error, #{msg => "unexpected call", req => Req}), {reply, ignored, State}. handle_cast(Req, State) -> - ?LOG(error, "Unexpected case: ~p", [Req]), + ?SLOG(error, #{msg => "unexpected cast", req => Req}), {noreply, State}. handle_info(Info, State) -> - ?LOG(error, "Unexpected info: ~p", [Info]), + ?SLOG(error, #{msg => "unexpected info", info => Info}), {noreply, State}. terminate(_Reason, _State) -> 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 363963f96..8659605c4 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl @@ -188,9 +188,9 @@ check_url(URL) -> end. check_body(Body) -> - lists:all(fun({_, V}) -> - is_binary(V) - end, maps:to_list(Body)). + maps:fold(fun(_K, _V, false) -> false; + (_K, V, true) -> is_binary(V) + end, true, Body). default_headers() -> maps:put(<<"content-type">>, diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_jwks_connector.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_jwks_connector.erl index d6e977be6..6ec57a4de 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_jwks_connector.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_jwks_connector.erl @@ -94,7 +94,9 @@ handle_info({http, {RequestID, Result}}, State1 = State0#{request_id := undefined}, case Result of {error, Reason} -> - ?LOG(error, "Failed to request jwks endpoint(~s): ~p", [Endpoint, Reason]), + ?SLOG(warning, #{msg => "failed to request jwks endpoint", + endpoint => Endpoint, + reason => Reason}), State1; {_StatusLine, _Headers, Body} -> try @@ -102,7 +104,9 @@ handle_info({http, {RequestID, Result}}, {_, JWKs} = JWKS#jose_jwk.keys, State1#{jwks := JWKs} catch _:_ -> - ?LOG(error, "Invalid jwks returned from jwks endpoint(~s): ~p~n", [Endpoint, Body]), + ?SLOG(warning, #{msg => "invalid jwks returned from jwks endpoint", + endpoint => Endpoint, + body => Body}), State1 end end; @@ -140,7 +144,9 @@ refresh_jwks(#{endpoint := Endpoint, NState = case httpc:request(get, {Endpoint, [{"Accept", "application/json"}]}, HTTPOpts, [{body_format, binary}, {sync, false}, {receiver, self()}]) of {error, Reason} -> - ?LOG(error, "Failed to request jwks endpoint(~s): ~p", [Endpoint, Reason]), + ?SLOG(warning, #{msg => "failed to request jwks endpoint", + endpoint => Endpoint, + reason => Reason}), State; {ok, RequestID} -> State#{request_id := RequestID} 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 d348a439c..4ae98d07d 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl @@ -146,14 +146,18 @@ authenticate(#{password := Password} = Credential, case emqx_resource:query(Unique, {find_one, Collection, Selector2, #{}}) of undefined -> ignore; {error, Reason} -> - ?LOG(error, "['~s'] Query failed: ~p", [Unique, Reason]), + ?SLOG(error, #{msg => "query failed", + unique => Unique, + reason => Reason}), ignore; Doc -> case check_password(Password, Doc, State) of ok -> {ok, #{is_superuser => is_superuser(Doc, State)}}; {error, {cannot_find_password_hash_field, PasswordHashField}} -> - ?LOG(error, "['~s'] Can't find password hash field: ~s", [Unique, PasswordHashField]), + ?SLOG(error, #{msg => "can't find password hash field", + unique => Unique, + password_hash_field => PasswordHashField}), {error, bad_username_or_password}; {error, Reason} -> {error, Reason} 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 5d73f9410..572ae9a2c 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl @@ -138,7 +138,9 @@ authenticate(#{password := Password} = Credential, {error, Reason} end; {error, Reason} -> - ?LOG(error, "['~s'] Query failed: ~p", [Unique, Reason]), + ?SLOG(error, #{msg => "query failed", + unique => Unique, + reason => Reason}), ignore end.