chore(authn): improve code

apps/emqx/src/emqx_authentication.erl

@@ -310,7 +310,7 @@ do_authenticate([#authenticator{id = ID, provider = Provider, state = State} | M
             {stop, Result}
     catch
         Class:Reason:Stacktrace ->
-            ?SLOG(warning, #{msg => "an unexpected error in authentication",
+            ?SLOG(warning, #{msg => "unexpected_error_in_authentication",
                              class => Class,
                              reason => Reason,
                              stacktrace => Stacktrace,

apps/emqx_authn/src/emqx_authn_utils.erl

@@ -18,6 +18,8 @@
 
 -export([ replace_placeholders/2
         , replace_placeholder/2
+        , check_password/3
+        , is_superuser/1
         , hash/4
         , gen_salt/0
         , bin/1
@@ -55,6 +57,28 @@ replace_placeholder(<<"${cert-common-name}">>, Credential) ->
 replace_placeholder(Constant, _) ->
     Constant.
 
+check_password(undefined, _Selected, _State) ->
+    {error, bad_username_or_password};
+check_password(Password,
+               #{<<"password_hash">> := Hash},
+               #{password_hash_algorithm := bcrypt}) ->
+    case {ok, Hash} =:= bcrypt:hashpw(Password, Hash) of
+        true -> ok;
+        false -> {error, bad_username_or_password}
+    end;
+check_password(Password,
+               #{<<"password_hash">> := Hash} = Selected,
+               #{password_hash_algorithm := Algorithm,
+                 salt_position := SaltPosition}) ->
+    Salt = maps:get(<<"salt">>, Selected, <<>>),
+    case Hash =:= hash(Algorithm, Password, Salt, SaltPosition) of
+        true -> ok;
+        false -> {error, bad_username_or_password}
+    end.
+
+is_superuser(Selected) ->
+    #{is_superuser => maps:get(<<"is_superuser">>, Selected, false)}.
+
 hash(Algorithm, Password, Salt, prefix) ->
     emqx_passwd:hash(Algorithm, <<Salt/binary, Password/binary>>);
 hash(Algorithm, Password, Salt, suffix) ->

apps/emqx_authn/src/simple_authn/emqx_authn_http.erl

@@ -165,11 +165,14 @@ authenticate(Credential, #{'_unique' := Unique,
                 {ok, NBody} ->
                     %% TODO: Return by user property
                     {ok, #{is_superuser => maps:get(<<"is_superuser">>, NBody, false),
-                            user_property => NBody}};
+                           user_property => NBody}};
                 {error, _Reason} ->
                     {ok, #{is_superuser => false}}
             end;
-        {error, _Reason} ->
+        {error, Reason} ->
+            ?SLOG(error, #{msg => "http_server_query_failed",
+                           resource => Unique,
+                           reason => Reason}),
             ignore
     end.
 

apps/emqx_authn/src/simple_authn/emqx_authn_jwks_connector.erl

@@ -94,7 +94,7 @@ handle_info({http, {RequestID, Result}},
     State1 = State0#{request_id := undefined},
     case Result of
         {error, Reason} ->
-            ?SLOG(warning, #{msg => "failed to request jwks endpoint",
+            ?SLOG(warning, #{msg => "failed_to_request_jwks_endpoint",
                              endpoint => Endpoint,
                              reason => Reason}),
             State1;
@@ -104,7 +104,7 @@ handle_info({http, {RequestID, Result}},
                 {_, JWKs} = JWKS#jose_jwk.keys,
                 State1#{jwks := JWKs}
             catch _:_ ->
-                ?SLOG(warning, #{msg => "invalid jwks returned from jwks endpoint",
+                ?SLOG(warning, #{msg => "invalid_jwks_returned",
                                  endpoint => Endpoint,
                                  body => Body}),
                 State1
@@ -140,11 +140,14 @@ handle_options(#{endpoint := Endpoint,
 
 refresh_jwks(#{endpoint := Endpoint,
                ssl_opts := SSLOpts} = State) ->
-    HTTPOpts = [{timeout, 5000}, {connect_timeout, 5000}, {ssl, SSLOpts}],
+    HTTPOpts = [ {timeout, 5000}
+               , {connect_timeout, 5000}
+               , {ssl, SSLOpts}
+               ],
     NState = case httpc:request(get, {Endpoint, [{"Accept", "application/json"}]}, HTTPOpts,
                                 [{body_format, binary}, {sync, false}, {receiver, self()}]) of
                  {error, Reason} ->
-                     ?SLOG(warning, #{msg => "failed to request jwks endpoint",
+                     ?SLOG(warning, #{msg => "failed_to_request_jwks_endpoint",
                                       endpoint => Endpoint,
                                       reason => Reason}),
                      State;

apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl

@@ -146,8 +146,8 @@ authenticate(#{password := Password} = Credential,
     case emqx_resource:query(Unique, {find_one, Collection, Selector2, #{}}) of
         undefined -> ignore;
         {error, Reason} ->
-            ?SLOG(error, #{msg => "query failed",
+            ?SLOG(error, #{msg => "mongodb_query_failed",
-                           unique => Unique,
+                           resource => Unique,
                            reason => Reason}),
             ignore;
         Doc ->
@@ -155,10 +155,10 @@ authenticate(#{password := Password} = Credential,
                 ok ->
                     {ok, #{is_superuser => is_superuser(Doc, State)}};
                 {error, {cannot_find_password_hash_field, PasswordHashField}} ->
-                    ?SLOG(error, #{msg => "can't find password hash field",
+                    ?SLOG(error, #{msg => "cannot_find_password_hash_field",
-                                   unique => Unique,
+                                   resource => Unique,
                                    password_hash_field => PasswordHashField}),
-                    {error, bad_username_or_password};
+                    ignore;
                 {error, Reason} ->
                     {error, Reason}
             end

apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl

@@ -119,13 +119,16 @@ authenticate(#{password := Password} = Credential,
         {ok, _Columns, []} -> ignore;
         {ok, Columns, Rows} ->
             Selected = maps:from_list(lists:zip(Columns, Rows)),
-            case check_password(Password, Selected, State) of
+            case emqx_authn_utils:check_password(Password, Selected, State) of
                 ok ->
-                    {ok, #{is_superuser => maps:get(<<"is_superuser">>, Selected, false)}};
+                    {ok, emqx_authn_utils:is_superuser(Selected)};
                 {error, Reason} ->
                     {error, Reason}
             end;
-        {error, _Reason} ->
+        {error, Reason} ->
+            ?SLOG(error, #{msg => "mysql_query_failed",
+                           resource => Unique,
+                           reason => Reason}),
             ignore
     end.
 
@@ -137,24 +140,24 @@ destroy(#{'_unique' := Unique}) ->
 %% Internal functions
 %%------------------------------------------------------------------------------
 
-check_password(undefined, _Selected, _State) ->
+% check_password(undefined, _Selected, _State) ->
-    {error, bad_username_or_password};
+%     {error, bad_username_or_password};
-check_password(Password,
+% check_password(Password,
-               #{<<"password_hash">> := Hash},
+%                #{<<"password_hash">> := Hash},
-               #{password_hash_algorithm := bcrypt}) ->
+%                #{password_hash_algorithm := bcrypt}) ->
-    case {ok, Hash} =:= bcrypt:hashpw(Password, Hash) of
+%     case {ok, Hash} =:= bcrypt:hashpw(Password, Hash) of
-        true -> ok;
+%         true -> ok;
-        false -> {error, bad_username_or_password}
+%         false -> {error, bad_username_or_password}
-    end;
+%     end;
-check_password(Password,
+% check_password(Password,
-               #{<<"password_hash">> := Hash} = Selected,
+%                #{<<"password_hash">> := Hash} = Selected,
-               #{password_hash_algorithm := Algorithm,
+%                #{password_hash_algorithm := Algorithm,
-                 salt_position := SaltPosition}) ->
+%                  salt_position := SaltPosition}) ->
-    Salt = maps:get(<<"salt">>, Selected, <<>>),
+%     Salt = maps:get(<<"salt">>, Selected, <<>>),
-    case Hash =:= emqx_authn_utils:hash(Algorithm, Password, Salt, SaltPosition) of
+%     case Hash =:= emqx_authn_utils:hash(Algorithm, Password, Salt, SaltPosition) of
-        true -> ok;
+%         true -> ok;
-        false -> {error, bad_username_or_password}
+%         false -> {error, bad_username_or_password}
-    end.
+%     end.
 
 %% TODO: Support prepare
 parse_query(Query) ->

apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl

@@ -109,13 +109,16 @@ authenticate(#{password := Password} = Credential,
         {ok, Columns, Rows} ->
             NColumns = [Name || #column{name = Name} <- Columns],
             Selected = maps:from_list(lists:zip(NColumns, Rows)),
-            case check_password(Password, Selected, State) of
+            case emqx_authn_utils:check_password(Password, Selected, State) of
                 ok ->
-                    {ok, #{is_superuser => maps:get(<<"is_superuser">>, Selected, false)}};
+                    {ok, emqx_authn_utils:is_superuser(Selected)};
                 {error, Reason} ->
                     {error, Reason}
             end;
-        {error, _Reason} ->
+        {error, Reason} ->
+            ?SLOG(error, #{msg => "postgresql_query_failed",
+                           resource => Unique,
+                           reason => Reason}),
             ignore
     end.
 
@@ -127,25 +130,6 @@ destroy(#{'_unique' := Unique}) ->
 %% Internal functions
 %%------------------------------------------------------------------------------
 
-check_password(undefined, _Selected, _State) ->
-    {error, bad_username_or_password};
-check_password(Password,
-               #{<<"password_hash">> := Hash},
-               #{password_hash_algorithm := bcrypt}) ->
-    case {ok, Hash} =:= bcrypt:hashpw(Password, Hash) of
-        true -> ok;
-        false -> {error, bad_username_or_password}
-    end;
-check_password(Password,
-               #{<<"password_hash">> := Hash} = Selected,
-               #{password_hash_algorithm := Algorithm,
-                 salt_position := SaltPosition}) ->
-    Salt = maps:get(<<"salt">>, Selected, <<>>),
-    case Hash =:= emqx_authn_utils:hash(Algorithm, Password, Salt, SaltPosition) of
-        true -> ok;
-        false -> {error, bad_username_or_password}
-    end.
-
 %% TODO: Support prepare
 parse_query(Query) ->
     case re:run(Query, ?RE_PLACEHOLDER, [global, {capture, all, binary}]) of

apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl

@@ -130,16 +130,22 @@ authenticate(#{password := Password} = Credential,
     NKey = binary_to_list(iolist_to_binary(replace_placeholders(Key, Credential))),
     case emqx_resource:query(Unique, {cmd, [Command, NKey | Fields]}) of
         {ok, Values} ->
-            Selected = merge(Fields, Values),
+            case merge(Fields, Values) of
-            case check_password(Password, Selected, State) of
+                #{<<"password_hash">> := _} = Selected ->
-                ok ->
+                    case emqx_authn_utils:check_password(Password, Selected, State) of
-                    {ok, #{is_superuser => maps:get("is_superuser", Selected, false)}};
+                        ok ->
-                {error, Reason} ->
+                            {ok, emqx_authn_utils:is_superuser(Selected)};
-                    {error, Reason}
+                        {error, Reason} ->
+                            {error, Reason}
+                    end;
+                _ ->
+                    ?SLOG(error, #{msg => "cannot_find_password_hash_field",
+                                   resource => Unique}),
+                    ignore
             end;
         {error, Reason} ->
-            ?SLOG(error, #{msg => "query failed",
+            ?SLOG(error, #{msg => "redis_query_failed",
-                           unique => Unique,
+                           resource => Unique,
                            reason => Reason}),
             ignore
     end.
@@ -205,27 +211,5 @@ merge(Fields, Value) when not is_list(Value) ->
     merge(Fields, [Value]);
 merge(Fields, Values) ->
     maps:from_list(
-        lists:filter(fun({_, V}) ->
+        [{list_to_binary(K), V}
-                         V =/= undefined
+            || {K, V} <- lists:zip(Fields, Values), V =/= undefined]).
-                     end, lists:zip(Fields, Values))).
-
-check_password(undefined, _Selected, _State) ->
-    {error, bad_username_or_password};
-check_password(Password,
-               #{"password_hash" := PasswordHash},
-               #{password_hash_algorithm := bcrypt}) ->
-    case {ok, PasswordHash} =:= bcrypt:hashpw(Password, PasswordHash) of
-        true -> ok;
-        false -> {error, bad_username_or_password}
-    end;
-check_password(Password,
-               #{"password_hash" := PasswordHash} = Selected,
-               #{password_hash_algorithm := Algorithm,
-                 salt_position := SaltPosition}) ->
-    Salt = maps:get("salt", Selected, <<>>),
-    case PasswordHash =:= emqx_authn_utils:hash(Algorithm, Password, Salt, SaltPosition) of
-        true -> ok;
-        false -> {error, bad_username_or_password}
-    end;
-check_password(_Password, _Selected, _State) ->
-    ignore.