From 9e97760520e675fed1617780604670b87c293769 Mon Sep 17 00:00:00 2001
From: firest <blankalupo@163.com>
Date: Wed, 14 Sep 2022 11:23:31 +0800
Subject: [PATCH] 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
         }
     ].