Merge pull request #13387 from zhongwencool/dont-override-authn-users

fix: don't override authn users when import_user from authn.boostrap_file

apps/emqx_auth/etc/auth-built-in-db-bootstrap.csv

@@ -0,0 +1 @@
+user_id,password,is_superuser

apps/emqx_auth/test/data/user-credentials-plain_v2.csv

@@ -0,0 +1,3 @@
+user_id,password,is_superuser
+myuser3,Password4,true
+myuser4,Password3,false

apps/emqx_auth_mnesia/src/emqx_authn_mnesia.erl

@@ -171,67 +171,61 @@ do_destroy(UserGroup) ->
         mnesia:select(?TAB, group_match_spec(UserGroup), write)
     ).
 
-import_users({PasswordType, Filename, FileData}, State) ->
+import_users(ImportSource, State) ->
+    import_users(ImportSource, State, #{override => true}).
+
+import_users({PasswordType, Filename, FileData}, State, Opts) ->
     Convertor = convertor(PasswordType, State),
-    try
-        {_NewUsersCnt, Users} = parse_import_users(Filename, FileData, Convertor),
-        case length(Users) > 0 andalso do_import_users(Users) of
-            false ->
-                error(empty_users);
-            ok ->
-                ok;
-            {error, Reason} ->
-                _ = do_clean_imported_users(Users),
-                error(Reason)
-        end
+    try parse_import_users(Filename, FileData, Convertor) of
+        Users ->
+            case do_import_users(Users, Opts#{filename => Filename}) of
+                ok ->
+                    ok;
+                %% Do not log empty user entries.
+                %% The default etc/auth-built-in-db.csv file contains an empty user entry.
+                {error, empty_users} ->
+                    {error, empty_users};
+                {error, Reason} ->
+                    ?SLOG(
+                        warning,
+                        #{
+                            msg => "import_authn_users_failed",
+                            reason => Reason,
+                            type => PasswordType,
+                            filename => Filename
+                        }
+                    ),
+                    {error, Reason}
+            end
     catch
-        error:Reason1:Stk ->
+        error:Reason:Stk ->
             ?SLOG(
                 warning,
                 #{
-                    msg => "import_users_failed",
-                    reason => Reason1,
+                    msg => "parse_authn_users_failed",
+                    reason => Reason,
                     type => PasswordType,
                     filename => Filename,
                     stacktrace => Stk
                 }
             ),
-            {error, Reason1}
+            {error, Reason}
     end.
 
-do_import_users(Users) ->
+do_import_users([], _Opts) ->
+    {error, empty_users};
+do_import_users(Users, Opts) ->
     trans(
         fun() ->
             lists:foreach(
-                fun(
-                    #{
-                        <<"user_group">> := UserGroup,
-                        <<"user_id">> := UserID,
-                        <<"password_hash">> := PasswordHash,
-                        <<"salt">> := Salt,
-                        <<"is_superuser">> := IsSuperuser
-                    }
-                ) ->
-                    insert_user(UserGroup, UserID, PasswordHash, Salt, IsSuperuser)
+                fun(User) ->
+                    insert_user(User, Opts)
                 end,
                 Users
             )
         end
     ).
 
-do_clean_imported_users(Users) ->
-    lists:foreach(
-        fun(
-            #{
-                <<"user_group">> := UserGroup,
-                <<"user_id">> := UserID
-            }
-        ) ->
-            mria:dirty_delete(?TAB, {UserGroup, UserID})
-        end,
-        Users
-    ).
-
 add_user(
     UserInfo,
     State
@@ -338,7 +332,14 @@ run_fuzzy_filter(
 %% Internal functions
 %%------------------------------------------------------------------------------
 
-insert_user(UserGroup, UserID, PasswordHash, Salt, IsSuperuser) ->
+insert_user(User, Opts) ->
+    #{
+        <<"user_group">> := UserGroup,
+        <<"user_id">> := UserID,
+        <<"password_hash">> := PasswordHash,
+        <<"salt">> := Salt,
+        <<"is_superuser">> := IsSuperuser
+    } = User,
     UserInfoRecord =
         #user_info{user_id = DBUserID} =
         user_info_record(UserGroup, UserID, PasswordHash, Salt, IsSuperuser),
@@ -348,14 +349,22 @@ insert_user(UserGroup, UserID, PasswordHash, Salt, IsSuperuser) ->
         [UserInfoRecord] ->
             ok;
         [_] ->
+            Msg =
+                case maps:get(override, Opts, false) of
+                    true ->
+                        insert_user(UserInfoRecord),
+                        "override_an_exists_userid_into_authentication_database_ok";
+                    false ->
+                        "import_an_exists_userid_into_authentication_database_failed"
+                end,
             ?SLOG(warning, #{
-                msg => "bootstrap_authentication_overridden_in_the_built_in_database",
+                msg => Msg,
                 user_id => UserID,
                 group_id => UserGroup,
+                bootstrap_file => maps:get(filename, Opts),
                 suggestion =>
-                    "If you have made changes in other way, remove the user_id from the bootstrap file."
-            }),
-            insert_user(UserInfoRecord)
+                    "If you've altered it differently, delete the user_id from the bootstrap file."
+            })
     end.
 
 insert_user(#user_info{} = UserInfoRecord) ->
@@ -473,27 +482,7 @@ parse_import_users(Filename, FileData, Convertor) ->
         end
     end,
     ReaderFn = reader_fn(Filename, FileData),
-    Users = Eval(ReaderFn),
-    NewUsersCount =
-        lists:foldl(
-            fun(
-                #{
-                    <<"user_group">> := UserGroup,
-                    <<"user_id">> := UserID
-                },
-                Acc
-            ) ->
-                case ets:member(?TAB, {UserGroup, UserID}) of
-                    true ->
-                        Acc;
-                    false ->
-                        Acc + 1
-                end
-            end,
-            0,
-            Users
-        ),
-    {NewUsersCount, Users}.
+    Eval(ReaderFn).
 
 reader_fn(prepared_user_list, List) when is_list(List) ->
     %% Example: [#{<<"user_id">> => <<>>, ...}]
@@ -511,7 +500,7 @@ reader_fn(Filename0, Data) ->
                     error(Reason)
             end;
         <<".csv">> ->
-            %% Example: data/user-credentials.csv
+            %% Example: etc/auth-built-in-db-bootstrap.csv
             emqx_utils_stream:csv(Data);
         <<>> ->
             error(unknown_file_format);
@@ -556,16 +545,15 @@ is_superuser(#{<<"is_superuser">> := true}) -> true;
 is_superuser(_) -> false.
 
 boostrap_user_from_file(Config, State) ->
-    case maps:get(boostrap_file, Config, <<>>) of
+    case maps:get(bootstrap_file, Config, <<>>) of
         <<>> ->
             ok;
         FileName0 ->
-            #{boostrap_type := Type} = Config,
+            #{bootstrap_type := Type} = Config,
             FileName = emqx_schema:naive_env_interpolation(FileName0),
             case file:read_file(FileName) of
                 {ok, FileData} ->
-                    %% if there is a key conflict, override with the key which from the bootstrap file
-                    _ = import_users({Type, FileName, FileData}, State),
+                    _ = import_users({Type, FileName, FileData}, State, #{override => false}),
                     ok;
                 {error, Reason} ->
                     ?SLOG(warning, #{

apps/emqx_auth_mnesia/src/emqx_authn_mnesia_schema.erl

@@ -46,7 +46,7 @@ select_union_member(_Kind, _Value) ->
 fields(builtin_db) ->
     [
         {password_hash_algorithm, fun emqx_authn_password_hashing:type_rw/1}
-    ] ++ common_fields() ++ bootstrap_fields();
+    ] ++ common_fields();
 fields(builtin_db_api) ->
     [
         {password_hash_algorithm, fun emqx_authn_password_hashing:type_rw_api/1}
@@ -68,7 +68,8 @@ common_fields() ->
         {mechanism, emqx_authn_schema:mechanism(?AUTHN_MECHANISM_SIMPLE)},
         {backend, emqx_authn_schema:backend(?AUTHN_BACKEND)},
         {user_id_type, fun user_id_type/1}
-    ] ++ emqx_authn_schema:common_fields().
+    ] ++ bootstrap_fields() ++
+        emqx_authn_schema:common_fields().
 
 bootstrap_fields() ->
     [
@@ -78,7 +79,7 @@ bootstrap_fields() ->
                 #{
                     desc => ?DESC(bootstrap_file),
                     required => false,
-                    default => <<>>
+                    default => <<"${EMQX_ETC_DIR}/auth-built-in-db-bootstrap.csv">>
                 }
             )},
         {bootstrap_type,

apps/emqx_auth_mnesia/test/emqx_authn_mnesia_SUITE.erl

@@ -56,6 +56,7 @@ t_create(_) ->
     Config1 = Config0#{password_hash_algorithm => #{name => sha256}},
     {ok, _} = emqx_authn_mnesia:create(?AUTHN_ID, Config1),
     ok.
+
 t_bootstrap_file(_) ->
     Config = config(),
     %% hash to hash
@@ -102,25 +103,39 @@ t_bootstrap_file(_) ->
         ],
         test_bootstrap_file(HashConfig, plain, <<"user-credentials-plain.json">>)
     ),
+    Opts = #{clean => false},
+    Result = test_bootstrap_file(HashConfig, plain, <<"user-credentials-plain.csv">>, Opts),
     ?assertMatch(
         [
             {user_info, {_, <<"myuser3">>}, _, _, true},
             {user_info, {_, <<"myuser4">>}, _, _, false}
         ],
-        test_bootstrap_file(HashConfig, plain, <<"user-credentials-plain.csv">>)
+        Result
+    ),
+    %% Don't override the exist user id.
+    ?assertMatch(
+        Result, test_bootstrap_file(HashConfig, plain, <<"user-credentials-plain_v2.csv">>)
     ),
     ok.
 
 test_bootstrap_file(Config0, Type, File) ->
+    test_bootstrap_file(Config0, Type, File, #{clean => true}).
+
+test_bootstrap_file(Config0, Type, File, Opts) ->
     {Type, Filename, _FileData} = sample_filename_and_data(Type, File),
     Config2 = Config0#{
-        boostrap_file => Filename,
-        boostrap_type => Type
+        bootstrap_file => Filename,
+        bootstrap_type => Type
     },
     {ok, State0} = emqx_authn_mnesia:create(?AUTHN_ID, Config2),
     Result = ets:tab2list(emqx_authn_mnesia),
-    ok = emqx_authn_mnesia:destroy(State0),
-    ?assertMatch([], ets:tab2list(emqx_authn_mnesia)),
+    case maps:get(clean, Opts) of
+        true ->
+            ok = emqx_authn_mnesia:destroy(State0),
+            ?assertMatch([], ets:tab2list(emqx_authn_mnesia));
+        _ ->
+            ok
+    end,
     Result.
 
 t_update(_) ->

rebar.config.erl

@@ -469,6 +469,8 @@ relx_overlay(ReleaseType, Edition) ->
         {copy, "bin/install_upgrade.escript", "bin/install_upgrade.escript-{{release_version}}"},
         {copy, "apps/emqx_gateway_lwm2m/lwm2m_xml", "etc/lwm2m_xml"},
         {copy, "apps/emqx_auth/etc/acl.conf", "etc/acl.conf"},
+        {copy, "apps/emqx_auth/etc/auth-built-in-db-bootstrap.csv",
+            "etc/auth-built-in-db-bootstrap.csv"},
         {template, "bin/emqx.cmd", "bin/emqx.cmd"},
         {template, "bin/emqx_ctl.cmd", "bin/emqx_ctl.cmd"},
         {copy, "bin/nodetool", "bin/nodetool"},

rel/i18n/emqx_authn_mnesia_schema.hocon

@@ -11,9 +11,8 @@ user_id_type.label:
 
 bootstrap_file.desc:
 """The bootstrap file imports users into the built-in database.
-The file content format is determined by `bootstrap_type`.
-Remove the item from the bootstrap file when you have made changes in other way,
-otherwise, after restarting, the bootstrap item will be overridden again."""
+It will not import a user ID that already exists in the database.
+The file content format is determined by `bootstrap_type`."""
 
 bootstrap_file.label:
 """Bootstrap File Path"""