Merge pull request #8956 from lafirest/fix/redis_authn

fix(authn_redis): Add new clause for non-existent key check

apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl

@@ -141,7 +141,8 @@ authenticate(
                 {ok, []} ->
                     ignore;
                 {ok, Values} ->
-                    Selected = merge(Fields, Values),
+                    case merge(Fields, Values) of
+                        Selected when Selected =/= #{} ->
                             case
                                 emqx_authn_utils:check_password_from_selected_map(
                                     Algorithm, Selected, Password
@@ -149,7 +150,16 @@ authenticate(
                             of
                                 ok ->
                                     {ok, emqx_authn_utils:is_superuser(Selected)};
-                        {error, _Reason} ->
+                                {error, _Reason} = Error ->
+                                    Error
+                            end;
+                        _ ->
+                            ?TRACE_AUTHN_PROVIDER(info, "redis_query_not_matched", #{
+                                resource => ResourceId,
+                                cmd => Command,
+                                keys => NKey,
+                                fields => Fields
+                            }),
                             ignore
                     end;
                 {error, Reason} ->

apps/emqx_authn/test/emqx_authn_redis_SUITE.erl

@@ -161,11 +161,13 @@ t_authenticate(_Config) ->
         user_seeds()
     ).
 
-test_user_auth(#{
+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 maps:get(redis_result, Config, undefined) of
-        case Result of
+        undefined ->
-            {error, _} ->
+            ok;
-                ignore;
+        RedisResult ->
-            Any ->
+            ?assertEqual(RedisResult, emqx_authn_redis:authenticate(Credentials, State))
-                Any
     end,
-    ?assertEqual(AuthnResult, emqx_authn_redis:authenticate(Credentials, State)),
 
     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
         }
     ].