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

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,57 @@ 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;
+                {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 +328,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 +345,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 +478,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">> => <<>>, ...}]
@@ -564,8 +549,7 @@ boostrap_user_from_file(Config, State) ->
             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/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,16 +103,25 @@ 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,
@@ -119,8 +129,13 @@ test_bootstrap_file(Config0, Type, File) ->
     },
     {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(_) ->

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"""