From 9e97760520e675fed1617780604670b87c293769 Mon Sep 17 00:00:00 2001 From: firest Date: Wed, 14 Sep 2022 11:23:31 +0800 Subject: [PATCH 1/3] fix(authn_redis): Add new clause for non-existent key check fix #8800 when the key not-existing, redis may return a list that all elements are `undefined` --- .../src/simple_authn/emqx_authn_redis.erl | 26 ++++++----- .../test/emqx_authn_redis_SUITE.erl | 45 +++++++++++++------ 2 files changed, 47 insertions(+), 24 deletions(-) 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 4cc00322f..c6d2846ab 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl @@ -141,16 +141,22 @@ authenticate( {ok, []} -> ignore; {ok, Values} -> - Selected = merge(Fields, Values), - case - emqx_authn_utils:check_password_from_selected_map( - Algorithm, Selected, Password - ) - of - ok -> - {ok, emqx_authn_utils:is_superuser(Selected)}; - {error, _Reason} -> - ignore + case lists:all(fun(E) -> E =:= undefined end, Values) of + true -> + %% key not exists + ignore; + _ -> + Selected = merge(Fields, Values), + case + emqx_authn_utils:check_password_from_selected_map( + Algorithm, Selected, Password + ) + of + ok -> + {ok, emqx_authn_utils:is_superuser(Selected)}; + {error, _Reason} = Error -> + Error + end end; {error, Reason} -> ?TRACE_AUTHN_PROVIDER(error, "redis_query_failed", #{ diff --git a/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl b/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl index f9ed8bcb1..cd89a7fa6 100644 --- a/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl +++ b/apps/emqx_authn/test/emqx_authn_redis_SUITE.erl @@ -161,11 +161,13 @@ t_authenticate(_Config) -> user_seeds() ). -test_user_auth(#{ - credentials := Credentials0, - config_params := SpecificConfigParams, - result := Result -}) -> +test_user_auth( + #{ + credentials := Credentials0, + config_params := SpecificConfigParams, + result := Result + } = Config +) -> AuthConfig = maps:merge(raw_redis_auth_config(), SpecificConfigParams), {ok, _} = emqx:update_config( @@ -183,14 +185,12 @@ test_user_auth(#{ ?assertEqual(Result, emqx_access_control:authenticate(Credentials)), - AuthnResult = - case Result of - {error, _} -> - ignore; - Any -> - Any - end, - ?assertEqual(AuthnResult, emqx_authn_redis:authenticate(Credentials, State)), + case maps:get(redis_result, Config, undefined) of + undefined -> + ok; + RedisResult -> + ?assertEqual(RedisResult, emqx_authn_redis:authenticate(Credentials, State)) + end, emqx_authn_test_lib:delete_authenticators( [authentication], @@ -478,7 +478,7 @@ user_seeds() -> <<"cmd">> => <<"HMGET mqtt_user:${username} password_hash salt is_superuser">>, <<"password_hash_algorithm">> => #{<<"name">> => <<"bcrypt">>} }, - result => {error, not_authorized} + result => {error, bad_username_or_password} }, #{ @@ -547,6 +547,23 @@ user_seeds() -> } }, result => {ok, #{is_superuser => true}} + }, + + %% user not exists + #{ + data => #{ + password_hash => <<"plainsalt">>, + salt => <<"salt">>, + is_superuser => <<"1">> + }, + credentials => #{ + username => <<"not_exists">>, + password => <<"plain">> + }, + key => <<"mqtt_user:plain">>, + config_params => #{}, + result => {error, not_authorized}, + redis_result => ignore } ]. From 62fd955a0ec4e384a63841aa0e4728caebf5b216 Mon Sep 17 00:00:00 2001 From: firest Date: Wed, 14 Sep 2022 14:16:43 +0800 Subject: [PATCH 2/3] fix(authn_redis): make dialyzer happy --- apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) 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 c6d2846ab..735cfe226 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl @@ -141,8 +141,8 @@ authenticate( {ok, []} -> ignore; {ok, Values} -> - case lists:all(fun(E) -> E =:= undefined end, Values) of - true -> + case check_query_matched(Values) of + false -> %% key not exists ignore; _ -> @@ -228,3 +228,10 @@ merge(Fields, Values) -> || {K, V} <- lists:zip(Fields, Values), V =/= undefined ] ). + +check_query_matched(undefined) -> + false; +check_query_matched(List) when is_list(List) -> + lists:any(fun(E) -> E =/= undefined end, List); +check_query_matched(_) -> + true. From 8590fef829a8ab611248d888c39d8dd1bc4ba713 Mon Sep 17 00:00:00 2001 From: firest Date: Wed, 14 Sep 2022 16:30:59 +0800 Subject: [PATCH 3/3] fix(authn_redis): Avoid duplicating check for non-existent keys --- .../src/simple_authn/emqx_authn_redis.erl | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) 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 735cfe226..215b05637 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl @@ -141,12 +141,8 @@ authenticate( {ok, []} -> ignore; {ok, Values} -> - case check_query_matched(Values) of - false -> - %% key not exists - ignore; - _ -> - Selected = merge(Fields, Values), + case merge(Fields, Values) of + Selected when Selected =/= #{} -> case emqx_authn_utils:check_password_from_selected_map( Algorithm, Selected, Password @@ -156,7 +152,15 @@ authenticate( {ok, emqx_authn_utils:is_superuser(Selected)}; {error, _Reason} = Error -> Error - end + end; + _ -> + ?TRACE_AUTHN_PROVIDER(info, "redis_query_not_matched", #{ + resource => ResourceId, + cmd => Command, + keys => NKey, + fields => Fields + }), + ignore end; {error, Reason} -> ?TRACE_AUTHN_PROVIDER(error, "redis_query_failed", #{ @@ -228,10 +232,3 @@ merge(Fields, Values) -> || {K, V} <- lists:zip(Fields, Values), V =/= undefined ] ). - -check_query_matched(undefined) -> - false; -check_query_matched(List) when is_list(List) -> - lists:any(fun(E) -> E =/= undefined end, List); -check_query_matched(_) -> - true.